Re: [PATCH v14 02/19] unwind_user/sframe: Add support for reading .sframe headers

On 5/9/2026 1:02 AM, Indu Bhagat wrote:
> On 2026-05-06 06:42, Jens Remus wrote:
>> On 5/5/2026 2:49 PM,[email protected] wrote:
>>> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>>> - [Medium] Missing validation of shdr.abi_arch allows parsing SFrame data from mismatched architectures.
>>> - [Medium] VMA addresses are incorrectly evaluated as boolean flags to check for zero-length inputs.
>>> - [Low] Mixing goto error handling with scoped_guard scope-based cleanup.
>>> -- 
>>>
>>> commit afe7c9eb6f0e6fc3f73d0b6a555f8d3926cf6ccf
>>> Author: Josh Poimboeuf<[email protected]>
>>>
>>> unwind_user/sframe: Add support for reading .sframe headers
>>>
>>> This commit introduces basic infrastructure for unwinding user space stacks
>>> with SFrame. It adds support for parsing the .sframe section header.
>>>
>>> Link:https://lore.kernel.org/all/ f27e8463783febfa0dabb0432a3dd6be8ad98412.1737511963.git.jpoimboe@kernel.org/
>>>
>>>> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
>>>> new file mode 100644
>>>> index 0000000000000..c128c1eeaca20
>>>> --- /dev/null
>>>> +++ b/kernel/unwind/sframe.c
>>> [ ... ]
>>>> +static int sframe_read_header(struct sframe_section *sec)
>>>> +{
>>>> +    unsigned long header_end, fdes_start, fdes_end, fres_start, fres_end;
>>>> +    struct sframe_header shdr;
>>>> +    unsigned int num_fdes;
>>>> +
>>>> +    if (copy_from_user(&shdr, (void __user *)sec->sframe_start, sizeof(shdr))) {
>>>> +        dbg("header usercopy failed\n");
>>>> +        return -EFAULT;
>>>> +    }
>>>> +
>>>> +    if (shdr.preamble.magic != SFRAME_MAGIC ||
>>>> +        shdr.preamble.version != SFRAME_VERSION_3 ||
>>>> +        !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
>>>> +        !(shdr.preamble.flags & SFRAME_F_FDE_FUNC_START_PCREL) ||
>>         shdr.abi_arch != sframe_abi_arch() ||
>>
>>>> +        shdr.auxhdr_len) {
>>>> +        dbg("bad/unsupported sframe header\n");
>>>> +        return -EINVAL;
>>>> +    }
>>> Should we also validate shdr.abi_arch to ensure it matches the expected
>>> architecture?
>>>
>>> If malformed or foreign-architecture SFrame data is passed in (for instance,
>>> with an unexpected endianness or register mapping), the kernel might interpret
>>> it incorrectly and generate invalid stack traces.
>>>
>>> Furthermore, accepting arbitrary values here could risk ABI calcification if
>>> userspace starts supplying invalid values that we later want to reject.
>> I could introduce an architecture-specific sframe_abi_arch() and update
>> the SFrame header validation as above.  For x86 the implementation in
>> in unwind_user_sframe.h could look as follows:
>>
>> #ifdef CONFIG_X86_64
>> #define sframe_abi_arch()    SFRAME_ABI_AMD64_ENDIAN_LITTLE
>> #endif
>>
>> Given sframe.o is only linked in if CONFIG_HAVE_UNWIND_USER_SFRAME and
>> that is only selected if X86_64 this should be fine.
>>
>> Do you see any value?
> 
> I dont see much value.  fs/binfmt_elf.c has done some admission control
> for the user binary already.  For bi-endian systems, this doesnt add
> value (wrt checking correct endianness).
> 
> For addressing the concern of a compromised SFrame section, we need to
> ensure checks at SFrame information read/validation time (which the code
> is doing already and additional checks that can be added are currently
> being discussed).  Checking for the abi_arch value does not necessarily safeguard much.

Good point!  I agree to omit validating shdr.abi_arch, given the ELF
architecture and endianness have already been validated.

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
[email protected] / [email protected]

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/