From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id B8929385AC2C; Fri, 14 Oct 2022 14:08:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B8929385AC2C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1665756491; bh=ThEXlTgqgjpR50MOAN7SDogjQ+VfkqJJkBBfJlvVQRE=; h=From:To:Subject:Date:In-Reply-To:References:From; b=S3O18ASnAC0Wlwkx278ezVANH9kP9JBH3VX0eVpGfV2jtcrRpjiSLabRzh2nfu3S8 /KOJ77TU6k5Ssv2IKXwKOqcAqP82IFu6v1Q/S5OMDA2eVK7C3yrPChZqYY8Nh/iE5s y5yRXrNmou2Z/h2o24C+qrMu281oig/OtwVd/BcE= From: "torbjorn.svensson at st dot com" To: gdb-prs@sourceware.org Subject: [Bug gdb/28549] ARM/Cortex-M: improper stack unwinding when the target is in lockup state Date: Fri, 14 Oct 2022 14:07:33 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gdb X-Bugzilla-Component: gdb X-Bugzilla-Version: 9.2 X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: torbjorn.svensson at st dot com X-Bugzilla-Status: ASSIGNED X-Bugzilla-Resolution: X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: luis.machado at arm dot com X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://sourceware.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://sourceware.org/bugzilla/show_bug.cgi?id=3D28549 --- Comment #48 from tomas.vanek at fbl dot cz --- (In reply to Torbj=C3=B6rn SVENSSON from comment #47) >=20 > 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 g= et > it merged after a review process? If it helps, I can help with adapting t= he > 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.=20 'Show frame unwinder name in signal name' gives user little bit better info than - 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 chan= ges are applicable. Back to 'ARM M lockup frame unwinder' and Torbj=C3=B6rn'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 c= ode (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 d= ata, 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 mutu= ally exclusive, no matter which one is sniffed first. I agree, without the second patch the lockup unwinder registration should precede the exception unwinde= r, 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 wit= hout 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 unw= ind 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=C3=B6rn SVENSSON --- (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.=20 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 in= fo > than - 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. >=20 > Back to 'ARM M lockup frame unwinder' and Torbj=C3=B6rn's comments Fredrik > forwarded me by mail: >=20 > > 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. >=20 > 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=20 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 happenin= g. >=20 > I just kept the format of original arm_m_addr_is_magic(). > Maybe keeping all tests for 'special' addresses together has some advanta= ge > in code readability. >=20 > > 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 >=20 > Well the lockup unwinder gets registered with the exception unwinder as t= he > first two unwinders. You might miss the code from 'Fixes for ARM M except= ion > 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. >=20 > I looked to the new code to support ARMv8 profile M security extension ju= st > 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 behaviou= r' > codes. On https://developer.arm.com/documentation/100701/0200/The-EXC-RETURN-regis= ter, 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 fram= es > possibly saved by the processor and stop as soon as a sniffer hits someth= ing > 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. --=20 You are receiving this mail because: You are on the CC list for the bug.=