[Bug gdb/28549] ARM/Cortex-M: improper stack unwinding when the target is in lockup state

["torbjorn.svensson at st dot com" <sourceware-bugzilla@sourceware.org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=28549

--- Comment #48 from tomas.vanek at fbl dot cz ---
(In reply to Torbjörn SVENSSON from comment #47)
> 
> From what I heard, the FSF copyright process has now completed.

Yes, it's finally done. Sorry, it was my fault: I replied to
copyright-clerk@fsf.org first instead of sending the signed form to
assign@gnu.org

> Tomas, can you please send your patch to the gdb-patches ML in order to get
> it merged after a review process? If it helps, I can help with adapting the
> patch for the review comments, but you need to send the initial version.

Will do.

The discussion here narrowed to 'ARM M lockup frame unwinder' patch. I
originally attached 3 patches, see above. TBH I never intended to use just
the unwinder without accompanying changes. 
'Show frame unwinder name in signal name' gives user little bit better info
than <signal handler called> - it might be mergeable (I didn't check, will do).
The rest of work in 'Fixes for ARM M exception frame unwinder' is probably
definitely unusable because of merging ARMv8 profile M security extension
support.
I need some time to check the new code and see if some parts of my old changes
are applicable.

Back to 'ARM M lockup frame unwinder' and Torbjörn's comments Fredrik forwarded
me by mail:

> 1. arm_m_lockup_prev_register
> I think arm_m_lockup_prev_register should have an assert statement as it
> should never be reached unless the sniffer is incorrectly implemented.

I think the same but I was not able to find the part of the general frame code
(frame-unwind.c, frame.c ...) which ensures it. That's why I wanted to avoid
gdb fail or assert because post-mortem debugging of frame related structures
seemed me almost impossible.
So if you confirm that prev_register method cannot be called when this_id
method returned outer_frame in any corner case like sniffing of corrupted data,
then ok, let's just assert(0) in prev_register

> 2. arm_m_lockup_unwind_sniffer
> I would have inlined the arm_m_addr_is_lockup function here as there is
> only one call to it and it would make it easier to see what is happening.

I just kept the format of original arm_m_addr_is_magic().
Maybe keeping all tests for 'special' addresses together has some advantage in
code readability.

> 3. registering the lockup unwinder
> I think it makes sense to have the lockup unwinder as early in the list
> as possible to avoid any of the other unwinders to preserve the state of
> the target as much as possible

Well the lockup unwinder gets registered with the exception unwinder as the
first two unwinders. You might miss the code from 'Fixes for ARM M exception
frame unwinder' - with this patch, lockup and exception are sniffed as mutually
exclusive, no matter which one is sniffed first. I agree, without the second
patch the lockup unwinder registration should precede the exception unwinder,
especially with ARMv8 profile M security extension with arm_m_addr_is_magic()
catching lockup magic codes.

I looked to the new code to support ARMv8 profile M security extension just
very shortly.
I wonder why arm_m_addr_is_magic() decoding differs so much in branches without
and with the security extension. The former decodes just the used special
addresses, the latter whole pages with lot of 'undefined behaviour' codes.
IMO the frame unwinding should proceed as long as sniffers encounter frames
possibly saved by the processor and stop as soon as a sniffer hits something
suspicious like a special address described as 'undefined behaviour' in
architecture manual. Stopping frame unwinding seems me much better than
generating nonsense frames from corrupted data.
Considering this I think we need another meta unwinder - or better said unwind
stopper for 'undefined behaviour'. Registered as the last one and catching
everything what all other sniffers refuse.
What do you think?

--- Comment #49 from Torbjörn SVENSSON <torbjorn.svensson at st dot com> ---
(In reply to tomas.vanek from comment #48)
> The discussion here narrowed to 'ARM M lockup frame unwinder' patch. I
> originally attached 3 patches, see above. TBH I never intended to use just
> the unwinder without accompanying changes. 

Right now, I'm more concerned about the endless(?) loop when the target is
in lockup state. I think we should first solve that problem and then look
into if there is improvements to do to make it more obvious for the user
what's going on.

> 'Show frame unwinder name in signal name' gives user little bit better info
> than <signal handler called> - it might be mergeable (I didn't check, will
> do).

I think this patch could be an improvement, but not as important as the
previous one. Lets focus on getting the other one in first, okay?

> The rest of work in 'Fixes for ARM M exception frame unwinder' is probably
> definitely unusable because of merging ARMv8 profile M security extension
> support.

I don't think any of the other changes are needed anymore.

> I need some time to check the new code and see if some parts of my old
> changes are applicable.
> 
> Back to 'ARM M lockup frame unwinder' and Torbjörn's comments Fredrik
> forwarded me by mail:
> 
> > 1. arm_m_lockup_prev_register
> > I think arm_m_lockup_prev_register should have an assert statement as it
> > should never be reached unless the sniffer is incorrectly implemented.
> 
> I think the same but I was not able to find the part of the general frame
> code (frame-unwind.c, frame.c ...) which ensures it. That's why I wanted to
> avoid gdb fail or assert because post-mortem debugging of frame related
> structures seemed me almost impossible.
> So if you confirm that prev_register method cannot be called when this_id
> method returned outer_frame in any corner case like sniffing of corrupted
> data, then ok, let's just assert(0) in prev_register

I just landed a patch that shows how you can use the 
frame_unwind_stop_reason hook to stop the unwinding. I think the same logic
could be used in your patch too.
Regarding the assurance, please see Pedro's reply to the very same question
here: https://sourceware.org/pipermail/gdb-patches/2022-October/192655.html

> > 2. arm_m_lockup_unwind_sniffer
> > I would have inlined the arm_m_addr_is_lockup function here as there is
> > only one call to it and it would make it easier to see what is happening.
> 
> I just kept the format of original arm_m_addr_is_magic().
> Maybe keeping all tests for 'special' addresses together has some advantage
> in code readability.
> 
> > 3. registering the lockup unwinder
> > I think it makes sense to have the lockup unwinder as early in the list
> > as possible to avoid any of the other unwinders to preserve the state of
> > the target as much as possible
> 
> Well the lockup unwinder gets registered with the exception unwinder as the
> first two unwinders. You might miss the code from 'Fixes for ARM M exception
> frame unwinder' - with this patch, lockup and exception are sniffed as
> mutually exclusive, no matter which one is sniffed first. I agree, without
> the second patch the lockup unwinder registration should precede the
> exception unwinder,
> especially with ARMv8 profile M security extension with
> arm_m_addr_is_magic() catching lockup magic codes.
> 
> I looked to the new code to support ARMv8 profile M security extension just
> very shortly.
> I wonder why arm_m_addr_is_magic() decoding differs so much in branches
> without and with the security extension. The former decodes just the used
> special addresses, the latter whole pages with lot of 'undefined behaviour'
> codes.

On https://developer.arm.com/documentation/100701/0200/The-EXC-RETURN-register,
it's written as that 0xffxxxxxx is an EXC_RETURN pattern, even if there are
combinations within that that is illegal.

> IMO the frame unwinding should proceed as long as sniffers encounter frames
> possibly saved by the processor and stop as soon as a sniffer hits something
> suspicious like a special address described as 'undefined behaviour' in
> architecture manual. Stopping frame unwinding seems me much better than
> generating nonsense frames from corrupted data.

I agree completely with this statement.

> Considering this I think we need another meta unwinder - or better said
> unwind stopper for 'undefined behaviour'. Registered as the last one and
> catching everything what all other sniffers refuse.
> What do you think?

I don't think we need another unwinder, but we might need to have some more
checks in the arm_m_exception_cache function to stop the unwind in case an
unsupported state is detected.

-- 
You are receiving this mail because:
You are on the CC list for the bug.