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

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

[Jens Remus]

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?

> [ ... ]
>> +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)
jremus@de.ibm.com / jremus@linux.ibm.com

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/

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

[Jens Remus]

On 5/6/2026 3:42 PM, Jens Remus wrote:
> On 5/5/2026 2:49 PM, sashiko-bot@kernel.org wrote:

>> [ ... ]
>>> +	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.

Let's do that.

> 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;

Obviously this whole approach does not work, as it causes the section
to get freed once set goes out of scope.  Just learned the hard way,
that I had not thought this through well enough...

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
jremus@de.ibm.com / jremus@linux.ibm.com

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/

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

[Indu Bhagat]

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.

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

[Jens Remus]

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,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.

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)
jremus@de.ibm.com / jremus@linux.ibm.com

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/