Re: [PATCH v14 07/19] unwind_user/sframe: Wire up unwind_user to sframe

[Indu Bhagat <ibhagatgnu@gmail.com>]

On 2026-05-07 09:18, Jens Remus wrote:
> On 5/5/2026 8:55 PM,sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> - [High] Unbounded linear scan of user-controlled sframe entries can cause a CPU hard lockup in NMI context.
>> - [Medium] The commit message claims to refactor `unwind_user_next_common()` by removing an unused `pt_regs` parameter, but this change is entirely absent from the code modifications.
>> --
>>
>> commit 2e1e17bbaa1c0426d41d66ce7930a944f3e86ae7
>> Author: Josh Poimboeuf<jpoimboe@kernel.org>
>>
>> unwind_user/sframe: Wire up unwind_user to sframe
>>
>> This commit hooks up the sframe infrastructure into the unwind_user interface
>> to support user-space stack unwinding via sframe. It adds the
>> UNWIND_USER_TYPE_SFRAME type and updates the unwind_user_next() logic to
>> attempt sframe unwinding when available.
>>
>>>      [ Jens Remus: Remove unused pt_regs from unwind_user_next_common() and
>>>      its callers.  Simplify unwind_user_next_sframe(). ]
>> This isn't a bug, but does this commit message match the changes?
>> Looking at the diff and the code, there are no modifications to
>> unwind_user_next_common() or its callers to remove a pt_regs parameter.
> Sashiko AI review failed to understand this commit message comment.
> 
>>> diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
>>> index 90ab3c1a205ef..1fb2724197332 100644
>>> --- a/kernel/unwind/user.c
>>> +++ b/kernel/unwind/user.c
>> [ ... ]
>>> @@ -82,6 +83,16 @@ static int unwind_user_next_fp(struct unwind_user_state *state)
>>>   	return unwind_user_next_common(state, &fp_frame);
>>>   }
>>>   
>>> +static int unwind_user_next_sframe(struct unwind_user_state *state)
>>> +{
>>> +	struct unwind_user_frame frame;
>>> +
>>> +	/* sframe expects the frame to be local storage */
>>> +	if (sframe_find(state->ip, &frame))
>> Can this sequence cause a hard lockup in NMI context?
>>
>> Since this unwinder is often called from NMI context (like perf), sframe_find()
>> eventually calls __find_fre() to search for Frame Row Entries. If __find_fre()
>> performs a linear scan over an unbounded, user-controlled number of entries
>> (fde->fres_num), it could spin for a long time without yielding.
>>
>> If a malicious userspace program maps a large VMA and provides hundreds of
>> millions of FREs, would this loop calling unsafe_get_user() on every
>> iteration tie up the CPU long enough in NMI context to trigger the hard
>> lockup detector?
> Please advise.
> 
> A user could craft .sframe with a large number of FDEs (up to UINT32_MAX)
> and large number of FREs per FDE (up to UINT16_MAX).  Each FDE could point
> at the same FDA and FREs to save section space.  Each FDE would require
> 16 bytes in the FDE table. All FDEs could share a single FDA of 5 bytes
> and share FREs of minimum 5 bytes each (when using the 16-bit start
> address offset to have UINT16_MAX FREs) in the FRE table.

For the case of malicious .sframe with large number of FDEs, wouldnt the 
existing checks of the various offsets being less than sframe section 
size suffice and the following in sframe_read_header suffice:

    if (fres_start < fdes_end || fres_end > sec->sframe_end)

For malicious num fres, should we add a check that a function's num fres 
is less than or equal to func size (available in the FDE index entry)? 
It is an approximate check, but should provide some boundary check.