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

[Indu Bhagat <ibhagatgnu@gmail.com>]

On 2026-05-06 06:42, Jens Remus wrote:
> On 5/5/2026 2:49 PM,sashiko-bot@kernel.org 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<jpoimboe@kernel.org>
>>
>> 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.