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

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?

> [ ... ]
>> +int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>> +		       unsigned long text_start, unsigned long text_end)
>> +{
>> +	struct vm_area_struct *sframe_vma, *text_vma;
>> +	struct mm_struct *mm = current->mm;
>> +	struct sframe_section *sec;
>> +	int ret;
>> +
>> +	if (!sframe_start || !sframe_end || !text_start || !text_end) {

	if (sframe_start >= sframe_end || text_start >= text_end) {
		dbg("invalid sframe/text address\n");

Do you agree?

> Is it safe to use VMA start addresses as boolean flags?
> 
> Since vm_start can legitimately be zero in certain configurations (if
> mmap_min_addr permits), evaluating !sframe_start or !text_start could
> incorrectly reject perfectly valid mappings at address 0.
> 
> Also, to check for zero-length sections, would it be better to explicitly use
> comparisons like sframe_start >= sframe_end instead?
> 
>> +		dbg("zero-length sframe/text address\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	scoped_guard(mmap_read_lock, mm) {
> [ ... ]
>> +	sec = kzalloc(sizeof(*sec), GFP_KERNEL);
>> +	if (!sec)
>> +		return -ENOMEM;
>> +
>> +	sec->sframe_start	= sframe_start;
>> +	sec->sframe_end		= sframe_end;
>> +	sec->text_start		= text_start;
>> +	sec->text_end		= text_end;
>> +
>> +	ret = sframe_read_header(sec);
>> +	if (ret)
>> +		goto err_free;
> 
> Is it acceptable to mix goto error handling with scoped_guard() cleanup
> helpers in the same function?
> 
> The kernel's cleanup subsystem guidelines generally suggest that usage of
> goto and cleanup helpers shouldn't be mixed in the same routine.

This references to the following comment in include/linux/cleanup.h:

  Lastly, given that the benefit of cleanup helpers is removal of
  "goto", and that the "goto" statement can jump between scopes, the
  expectation is that usage of "goto" and cleanup helpers is never
  mixed in the same function. I.e. for a given routine, convert all
  resources that need a "goto" cleanup to scope-based cleanup, or
  convert none of them.

> 
> Could sec be allocated using __free(kfree) to avoid the goto entirely?

The goto error handling is used outside the scoped_guard().  So I think
it is fine.  Do you see any value in converting it as follows (my naive
attempt):

diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
@@ -722,13 +722,14 @@ static int sframe_read_header(struct sframe_section *sec)
        return 0;
 }

+DEFINE_FREE(free_section, struct sframe_section *, if (_T) free_section(_T))
 int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
                       unsigned long text_start, unsigned long text_end)
 {
        struct maple_tree *sframe_mt = &current->mm->sframe_mt;
        struct vm_area_struct *sframe_vma, *text_vma;
        struct mm_struct *mm = current->mm;
-       struct sframe_section *sec;
+       struct sframe_section *sec __free(free_section) = NULL;
        int ret;

        if (sframe_start >= sframe_end || text_start >= text_end) {
@@ -768,7 +769,7 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
        ret = sframe_read_header(sec);
        if (ret) {
                dbg_print_header(sec);
-               goto err_free;
+               return ret;
        }

        ret = sframe_validate_section(sec);
@@ -784,10 +785,6 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
        }

        return 0;
-
-err_free:
-       free_section(sec);
-       return ret;
 }

> 
>> +
>> +	/* TODO nowhere to store it yet - just free it and return an error */
>> +	ret = -ENOSYS;
>> +
>> +err_free:
>> +	free_section(sec);
>> +	return ret;
>> +}
> 

Thanks and 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/