Re: [PATCH v14 05/19] unwind_user/sframe: Add support for reading .sframe contents

Re: [PATCH v14 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Jens Remus]

On 5/5/2026 8:59 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [Critical] Division by zero in `__find_fre` when `fde->rep_size` is 0.
> - [High] `mtree_insert_range` is called with an exclusive end boundary, preventing contiguous SFrame sections.
> - [High] Unbounded linear search over user-controlled memory in `__find_fre` can cause hard lockups in atomic context.
> - [High] Integer overflows in SFrame header parsing allow bypass of bounds checks on 32-bit architectures.
> - [Medium] Generic SFrame unwinding code uses `unsafe_get_user()` for unaligned accesses, breaking strict-alignment architectures.
> --
> 
> commit 159c92acf3c961aecc6383f6c3f35a955591abf3
> Author: Josh Poimboeuf <jpoimboe@kernel.org>
> 
> unwind_user/sframe: Add support for reading .sframe contents
> 
> This commit adds support for reading .sframe section contents to facilitate
> user space stack unwinding. It introduces the sframe_find() interface which
> uses user_read_access_begin() and unsafe_*() accessors for performance.
> Support is included for SFrame V3, internal FDE representations, and
> PC-relative offsets.
> 
> Link: https://lore.kernel.org/all/77c0d1ec143bf2a53d66c4ecb190e7e0a576fbfd.1737511963.git.jpoimboe@kernel.org/
> Link: https://lore.kernel.org/all/b35ca3a3-8de5-4d32-8d30-d4e562f6b0de@linux.ibm.com/
> 
>> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
>> index fdb2a49085638..c60aa527984a1 100644
>> --- a/kernel/unwind/sframe.c
>> +++ b/kernel/unwind/sframe.c
>> @@ -15,9 +15,338 @@
>>  #include <linux/unwind_user_types.h>
>>  
>>  #include "sframe.h"
>> +#include "sframe_debug.h"
> 
> [ ... ]
> 
>> +static __always_inline int __read_fre(struct sframe_section *sec,
>> +				      struct sframe_fde_internal *fde,
>> +				      unsigned long fre_addr,
>> +				      struct sframe_fre_internal *fre)
>> +{
> [ ... ]
>> +	if (fre_addr + addr_size + 1 > sec->fres_end)
>> +		return -EFAULT;
>> +
>> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
> 
> Will this cause alignment faults on architectures with strict alignment
> requirements?
> 
> The .sframe format uses packed structures and variable-length datawords. The
> cur pointer might be unaligned here, and UNSAFE_GET_USER_INC() performs
> 16-bit or 32-bit reads via unsafe_get_user().

IIUC this should not be an issue for x86-64, s390, and arm64.

> [ ... ]
> 
>> +static __always_inline int __find_fre(struct sframe_section *sec,
>> +				      struct sframe_fde_internal *fde,
>> +				      unsigned long ip,
>> +				      struct unwind_user_frame *frame)
>> +{
>> +	unsigned char fde_pctype = SFRAME_V3_FDE_PCTYPE(fde->info);
>> +	struct sframe_fre_internal *fre, *prev_fre = NULL;
>> +	struct sframe_fre_internal fres[2];
>> +	unsigned long fre_addr;
>> +	bool which = false;
>> +	unsigned int i;
>> +	u32 ip_off;
>> +
>> +	ip_off = ip - fde->func_addr;
>> +
>> +	if (fde_pctype == SFRAME_FDE_PCTYPE_MASK)
>> +		ip_off %= fde->rep_size;
> 
> Can a malicious user provide an SFrame section with a rep_size of 0?
> 
> Since fde->rep_size is populated directly from the user-provided
> _fda.rep_size without validation, a 0 value here would trigger a
> divide-by-zero exception and a kernel panic.

Good catch!  I would add a check in __read_fde():

diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
@@ -67,6 +67,7 @@ static __always_inline int __read_fde(struct sframe_section *sec,
        unsigned long fde_addr, fda_addr, func_addr;
        struct sframe_fde_v3 _fde;
        struct sframe_fda_v3 _fda;
+       unsigned char fde_pctype;

        fde_addr = sec->fdes_start + (fde_num * sizeof(struct sframe_fde_v3));
        unsafe_copy_from_user(&_fde, (void __user *)fde_addr,
@@ -82,6 +83,10 @@ static __always_inline int __read_fde(struct sframe_section *sec,
        unsafe_copy_from_user(&_fda, (void __user *)fda_addr,
                              sizeof(struct sframe_fda_v3), Efault);

+       fde_pctype = SFRAME_V3_FDE_PCTYPE(_fda.info);
+       if (fde_pctype == SFRAME_FDE_PCTYPE_MASK && !_fda.rep_size)
+               return -EINVAL;
+
        fde->func_addr  = func_addr;
        fde->func_size  = _fde.func_size;
        fde->fda_off    = _fde.fres_off;

> 
>> +
>> +	fre_addr = sec->fres_start + fde->fres_off;
>> +
>> +	for (i = 0; i < fde->fres_num; i++) {
> 
> Can this loop cause a hard lockup in atomic context?
> 
> fde->fres_num is a 32-bit value copied from user space without validation.
> Since sframe_find() is designed to be called by unwinders in NMI context, an
> attacker could provide a very large number of valid entries. Executing
> billions of iterations and unsafe_get_user() calls could stall the CPU
> and trigger the hard lockup detector.

Please advise.

> 
>> +		int ret;
>> +
> 
> [ ... ]
> 
>> @@ -120,8 +449,10 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>>  	sec->text_end		= text_end;
>>  
>>  	ret = sframe_read_header(sec);
>> -	if (ret)
>> +	if (ret) {
>> +		dbg_print_header(sec);
>>  		goto err_free;
>> +	}
> 
> Can shdr.fre_len cause an integer overflow on 32-bit architectures during
> header parsing?
> 
> If a malicious user provides a large fre_len in the header, fres_end
> (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
> address space. This would bypass the bounds check in sframe_read_header(),
> allowing fres_start and fdes_start to point into kernel memory. Later, when
> __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
> arbitrary kernel memory disclosure.

SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
Do we need to add the following to the header parsing?

if (fdes_start >= fdes_end || fres_start >= fres_end) {
	dbg_sec("inconsistent FDE/FRE start/end address\n");
	return -EINVAL;
}

> 
>>  
>>  	ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end, sec, GFP_KERNEL);
> 
> Does passing sec->text_end directly as the last parameter to
> mtree_insert_range() break contiguous mappings?
> 
> mtree_insert_range() expects the last boundary to be inclusive, but
> sec->text_end represents the exclusive end address of the executable segment.
> If user space maps seamlessly contiguous text segments, the insertion for the
> second segment might overlap with the claimed end of the first, causing it to
> fail with -EEXIST.

Addressed in previous patch.

> 
>>  	if (ret) {
>>  		dbg("mtree_insert_range failed: text=%lx-%lx\n",
> 

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 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Steven Rostedt]

On Wed, 6 May 2026 16:34:34 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> >> +static __always_inline int __read_fre(struct sframe_section *sec,
> >> +				      struct sframe_fde_internal *fde,
> >> +				      unsigned long fre_addr,
> >> +				      struct sframe_fre_internal *fre)
> >> +{  
> > [ ... ]  
> >> +	if (fre_addr + addr_size + 1 > sec->fres_end)
> >> +		return -EFAULT;
> >> +
> >> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);  
> > 
> > Will this cause alignment faults on architectures with strict alignment
> > requirements?
> > 
> > The .sframe format uses packed structures and variable-length datawords. The
> > cur pointer might be unaligned here, and UNSAFE_GET_USER_INC() performs
> > 16-bit or 32-bit reads via unsafe_get_user().  
> 
> IIUC this should not be an issue for x86-64, s390, and arm64.

Do we have a way to make sure that sframe support will always be for
architectures that can handle alignment issues like this? There should
be something to force this via configs or something that will trigger a
warning or bug if this is built for architectures that can't handle
this alignment.

> >   
> >> +
> >> +	fre_addr = sec->fres_start + fde->fres_off;
> >> +
> >> +	for (i = 0; i < fde->fres_num; i++) {  
> > 
> > Can this loop cause a hard lockup in atomic context?
> > 
> > fde->fres_num is a 32-bit value copied from user space without validation.
> > Since sframe_find() is designed to be called by unwinders in NMI context, an

What? No. This looks to be a hallucination. sframe_find() will never be
called in NMI context. In fact, it can only be called in task context.

> > attacker could provide a very large number of valid entries. Executing
> > billions of iterations and unsafe_get_user() calls could stall the CPU
> > and trigger the hard lockup detector.  
> 
> Please advise.

That said, we should verify that fde->fres_num is at least always
smaller than the size of the table.

> 
> >   
> >> +		int ret;
> >> +  
> > 
> > [ ... ]
> >   
> >> @@ -120,8 +449,10 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
> >>  	sec->text_end		= text_end;
> >>  
> >>  	ret = sframe_read_header(sec);
> >> -	if (ret)
> >> +	if (ret) {
> >> +		dbg_print_header(sec);
> >>  		goto err_free;
> >> +	}  
> > 
> > Can shdr.fre_len cause an integer overflow on 32-bit architectures during
> > header parsing?
> > 
> > If a malicious user provides a large fre_len in the header, fres_end
> > (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
> > address space. This would bypass the bounds check in sframe_read_header(),
> > allowing fres_start and fdes_start to point into kernel memory. Later, when
> > __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
> > arbitrary kernel memory disclosure.  
> 
> SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
> arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
> Do we need to add the following to the header parsing?
> 
> if (fdes_start >= fdes_end || fres_start >= fres_end) {
> 	dbg_sec("inconsistent FDE/FRE start/end address\n");
> 	return -EINVAL;
> }

I guess this wouldn't hurt.

> 
> >   
> >>  
> >>  	ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end, sec, GFP_KERNEL);  
> > 
> > Does passing sec->text_end directly as the last parameter to
> > mtree_insert_range() break contiguous mappings?
> > 
> > mtree_insert_range() expects the last boundary to be inclusive, but
> > sec->text_end represents the exclusive end address of the executable segment.
> > If user space maps seamlessly contiguous text segments, the insertion for the
> > second segment might overlap with the claimed end of the first, causing it to
> > fail with -EEXIST.  
> 
> Addressed in previous patch.

And I just sent a patch to fix the documentation of
mtree_insert_range() to update the kerneldoc to explicitly state it is
inclusive :-p

-- Steve


> 
> >   
> >>  	if (ret) {
> >>  		dbg("mtree_insert_range failed: text=%lx-%lx\n",  
> >   
> 
> Thanks and regards,
> Jens

Re: [PATCH v14 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Jens Remus]

On 5/6/2026 5:01 PM, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:34:34 +0200
> Jens Remus <jremus@linux.ibm.com> wrote:
> 
>>>> +static __always_inline int __read_fre(struct sframe_section *sec,
>>>> +				      struct sframe_fde_internal *fde,
>>>> +				      unsigned long fre_addr,
>>>> +				      struct sframe_fre_internal *fre)
>>>> +{  
>>> [ ... ]  
>>>> +	if (fre_addr + addr_size + 1 > sec->fres_end)
>>>> +		return -EFAULT;
>>>> +
>>>> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);  
>>>
>>> Will this cause alignment faults on architectures with strict alignment
>>> requirements?
>>>
>>> The .sframe format uses packed structures and variable-length datawords. The
>>> cur pointer might be unaligned here, and UNSAFE_GET_USER_INC() performs
>>> 16-bit or 32-bit reads via unsafe_get_user().  
>>
>> IIUC this should not be an issue for x86-64, s390, and arm64.
> 
> Do we have a way to make sure that sframe support will always be for
> architectures that can handle alignment issues like this? There should
> be something to force this via configs or something that will trigger a
> warning or bug if this is built for architectures that can't handle
> this alignment.

Any suggestions are very welcome.

>>>> +
>>>> +	fre_addr = sec->fres_start + fde->fres_off;
>>>> +
>>>> +	for (i = 0; i < fde->fres_num; i++) {  
>>>
>>> Can this loop cause a hard lockup in atomic context?
>>>
>>> fde->fres_num is a 32-bit value copied from user space without validation.
>>> Since sframe_find() is designed to be called by unwinders in NMI context, an
> 
> What? No. This looks to be a hallucination. sframe_find() will never be
> called in NMI context. In fact, it can only be called in task context.
> 
>>> attacker could provide a very large number of valid entries. Executing
>>> billions of iterations and unsafe_get_user() calls could stall the CPU
>>> and trigger the hard lockup detector.  
>>
>> Please advise.
> 
> That said, we should verify that fde->fres_num is at least always
> smaller than the size of the table.

If we carry the .sframe header sfh->num_fres over to a new sec->num_fres
then __read_fde() could check:

	if (_fda.fres_num > sec->num_fres)
		return -EINVAL;

But I am not sure if that check provides much benefit, as fde->fres_num
is relative to fde->fres_off, which points into the
[sec->fres_start, sec->fres_end[ part of the .sframe section.
So even a small fde->fres_num might be out of range.  Given the FRE are
variable size, it is not easily possible to check upfront in
__read_fde() whether the fde->fres_num is valid.

The validity is checked later in __read_fre() before any fields are read
from the FRE, with fre_addr = sec->fres_start + fde->fres_off being
passed as argument:

	if (fre_addr + addr_size + 1 > sec->fres_end)
		return -EFAULT;

	[...]

	if (cur + (dataword_count * dataword_size) > sec->fres_end)
		return -EFAULT;

> 
>>
>>>   
>>>> +		int ret;
>>>> +  
>>>
>>> [ ... ]
>>>   
>>>> @@ -120,8 +449,10 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>>>>  	sec->text_end		= text_end;
>>>>  
>>>>  	ret = sframe_read_header(sec);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>> +		dbg_print_header(sec);
>>>>  		goto err_free;
>>>> +	}  
>>>
>>> Can shdr.fre_len cause an integer overflow on 32-bit architectures during
>>> header parsing?
>>>
>>> If a malicious user provides a large fre_len in the header, fres_end
>>> (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
>>> address space. This would bypass the bounds check in sframe_read_header(),
>>> allowing fres_start and fdes_start to point into kernel memory. Later, when
>>> __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
>>> arbitrary kernel memory disclosure.  
>>
>> SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
>> arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
>> Do we need to add the following to the header parsing?
>>
>> if (fdes_start >= fdes_end || fres_start >= fres_end) {
>> 	dbg_sec("inconsistent FDE/FRE start/end address\n");
>> 	return -EINVAL;
>> }
> 
> I guess this wouldn't hurt.

Ok.

>>>>  
>>>>  	ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end, sec, GFP_KERNEL);  
>>>
>>> Does passing sec->text_end directly as the last parameter to
>>> mtree_insert_range() break contiguous mappings?
>>>
>>> mtree_insert_range() expects the last boundary to be inclusive, but
>>> sec->text_end represents the exclusive end address of the executable segment.
>>> If user space maps seamlessly contiguous text segments, the insertion for the
>>> second segment might overlap with the claimed end of the first, causing it to
>>> fail with -EEXIST.  
>>
>> Addressed in previous patch.
> 
> And I just sent a patch to fix the documentation of
> mtree_insert_range() to update the kerneldoc to explicitly state it is
> inclusive :-p

Thanks!

>>
>>>   
>>>>  	if (ret) {
>>>>  		dbg("mtree_insert_range failed: text=%lx-%lx\n",  
>>>   
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 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Jens Remus]

On 5/6/2026 5:01 PM, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:34:34 +0200
> Jens Remus <jremus@linux.ibm.com> wrote:

>>> If a malicious user provides a large fre_len in the header, fres_end
>>> (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
>>> address space. This would bypass the bounds check in sframe_read_header(),
>>> allowing fres_start and fdes_start to point into kernel memory. Later, when
>>> __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
>>> arbitrary kernel memory disclosure.  
>>
>> SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
>> arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
>> Do we need to add the following to the header parsing?
>>
>> if (fdes_start >= fdes_end || fres_start >= fres_end) {
>> 	dbg_sec("inconsistent FDE/FRE start/end address\n");
>> 	return -EINVAL;
>> }
> 
> I guess this wouldn't hurt.

Reviewing my suggestion again I realize that this check would be
superfluous.  The existing computation and check already ensures that
the FDE table is within sframe section, the FRE table is within sframe
section, and both tables do not overlap:

	num_fdes   = shdr.num_fdes;
	fdes_start = header_end + shdr.fdes_off;
	fdes_end   = fdes_start + (num_fdes * sizeof(struct sframe_fde_v3));

	fres_start = header_end + shdr.fres_off;
	fres_end   = fres_start + shdr.fre_len;

	if (fres_start < fdes_end || fres_end > sec->sframe_end) {
		dbg_sec("inconsistent FDE/FRE offsets\n");
		return -EINVAL;
	}

- fdes_start and fres_start are computed from header_start and thus must
  be larger sframe_start
- fdes_end and fres_end are computed from their fdes_start and
  fres_start and thus must be larger than sframe_start
- fres_start < fdes_end ensures that the FDE table and FRE table do not
  overlap
- fres_end > sec->sframe_end ensures that fres_end (and fdes_end and both
  fdes_start and fres_start) are smaller or equal sframe_end

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 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Indu Bhagat]

On 2026-05-06 07:34, Jens Remus wrote:
> On 5/5/2026 8:59 PM, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>> - [Critical] Division by zero in `__find_fre` when `fde->rep_size` is 0.
>> - [High] `mtree_insert_range` is called with an exclusive end boundary, preventing contiguous SFrame sections.
>> - [High] Unbounded linear search over user-controlled memory in `__find_fre` can cause hard lockups in atomic context.
>> - [High] Integer overflows in SFrame header parsing allow bypass of bounds checks on 32-bit architectures.
>> - [Medium] Generic SFrame unwinding code uses `unsafe_get_user()` for unaligned accesses, breaking strict-alignment architectures.
>> --
>>
>> commit 159c92acf3c961aecc6383f6c3f35a955591abf3
>> Author: Josh Poimboeuf <jpoimboe@kernel.org>
>>
>> unwind_user/sframe: Add support for reading .sframe contents
>>
>> This commit adds support for reading .sframe section contents to facilitate
>> user space stack unwinding. It introduces the sframe_find() interface which
>> uses user_read_access_begin() and unsafe_*() accessors for performance.
>> Support is included for SFrame V3, internal FDE representations, and
>> PC-relative offsets.
>>
>> Link: https://lore.kernel.org/all/77c0d1ec143bf2a53d66c4ecb190e7e0a576fbfd.1737511963.git.jpoimboe@kernel.org/
>> Link: https://lore.kernel.org/all/b35ca3a3-8de5-4d32-8d30-d4e562f6b0de@linux.ibm.com/
>>
>>> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
>>> index fdb2a49085638..c60aa527984a1 100644
>>> --- a/kernel/unwind/sframe.c
>>> +++ b/kernel/unwind/sframe.c
>>> @@ -15,9 +15,338 @@
>>>   #include <linux/unwind_user_types.h>
>>>   
>>>   #include "sframe.h"
>>> +#include "sframe_debug.h"
>>
>> [ ... ]
>>
>>> +static __always_inline int __read_fre(struct sframe_section *sec,
>>> +				      struct sframe_fde_internal *fde,
>>> +				      unsigned long fre_addr,
>>> +				      struct sframe_fre_internal *fre)
>>> +{
>> [ ... ]
>>> +	if (fre_addr + addr_size + 1 > sec->fres_end)
>>> +		return -EFAULT;
>>> +
>>> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);
>>
>> Will this cause alignment faults on architectures with strict alignment
>> requirements?
>>
>> The .sframe format uses packed structures and variable-length datawords. The
>> cur pointer might be unaligned here, and UNSAFE_GET_USER_INC() performs
>> 16-bit or 32-bit reads via unsafe_get_user().
> 
> IIUC this should not be an issue for x86-64, s390, and arm64.
> 
>> [ ... ]
>>
>>> +static __always_inline int __find_fre(struct sframe_section *sec,
>>> +				      struct sframe_fde_internal *fde,
>>> +				      unsigned long ip,
>>> +				      struct unwind_user_frame *frame)
>>> +{
>>> +	unsigned char fde_pctype = SFRAME_V3_FDE_PCTYPE(fde->info);
>>> +	struct sframe_fre_internal *fre, *prev_fre = NULL;
>>> +	struct sframe_fre_internal fres[2];
>>> +	unsigned long fre_addr;
>>> +	bool which = false;
>>> +	unsigned int i;
>>> +	u32 ip_off;
>>> +
>>> +	ip_off = ip - fde->func_addr;
>>> +
>>> +	if (fde_pctype == SFRAME_FDE_PCTYPE_MASK)
>>> +		ip_off %= fde->rep_size;
>>
>> Can a malicious user provide an SFrame section with a rep_size of 0?
>>
>> Since fde->rep_size is populated directly from the user-provided
>> _fda.rep_size without validation, a 0 value here would trigger a
>> divide-by-zero exception and a kernel panic.
> 
> Good catch!  I would add a check in __read_fde():
> 
> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> @@ -67,6 +67,7 @@ static __always_inline int __read_fde(struct sframe_section *sec,
>          unsigned long fde_addr, fda_addr, func_addr;
>          struct sframe_fde_v3 _fde;
>          struct sframe_fda_v3 _fda;
> +       unsigned char fde_pctype;
> 
>          fde_addr = sec->fdes_start + (fde_num * sizeof(struct sframe_fde_v3));
>          unsafe_copy_from_user(&_fde, (void __user *)fde_addr,
> @@ -82,6 +83,10 @@ static __always_inline int __read_fde(struct sframe_section *sec,
>          unsafe_copy_from_user(&_fda, (void __user *)fda_addr,
>                                sizeof(struct sframe_fda_v3), Efault);
> 
> +       fde_pctype = SFRAME_V3_FDE_PCTYPE(_fda.info);
> +       if (fde_pctype == SFRAME_FDE_PCTYPE_MASK && !_fda.rep_size)
> +               return -EINVAL;
> +
>          fde->func_addr  = func_addr;
>          fde->func_size  = _fde.func_size;
>          fde->fda_off    = _fde.fres_off;
> 

Yes, I think this is good to add.

>>
>>> +
>>> +	fre_addr = sec->fres_start + fde->fres_off;
>>> +
>>> +	for (i = 0; i < fde->fres_num; i++) {
>>
>> Can this loop cause a hard lockup in atomic context?
>>
>> fde->fres_num is a 32-bit value copied from user space without validation.
>> Since sframe_find() is designed to be called by unwinders in NMI context, an
>> attacker could provide a very large number of valid entries. Executing
>> billions of iterations and unsafe_get_user() calls could stall the CPU
>> and trigger the hard lockup detector.
> 
> Please advise.
> 
>>
>>> +		int ret;
>>> +
>>
>> [ ... ]
>>
>>> @@ -120,8 +449,10 @@ int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
>>>   	sec->text_end		= text_end;
>>>   
>>>   	ret = sframe_read_header(sec);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		dbg_print_header(sec);
>>>   		goto err_free;
>>> +	}
>>
>> Can shdr.fre_len cause an integer overflow on 32-bit architectures during
>> header parsing?
>>
>> If a malicious user provides a large fre_len in the header, fres_end
>> (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
>> address space. This would bypass the bounds check in sframe_read_header(),
>> allowing fres_start and fdes_start to point into kernel memory. Later, when
>> __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
>> arbitrary kernel memory disclosure.
> 
> SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
> arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
> Do we need to add the following to the header parsing?
> 
> if (fdes_start >= fdes_end || fres_start >= fres_end) {
> 	dbg_sec("inconsistent FDE/FRE start/end address\n");
> 	return -EINVAL;
> }
> 
>>
>>>   
>>>   	ret = mtree_insert_range(sframe_mt, sec->text_start, sec->text_end, sec, GFP_KERNEL);
>>
>> Does passing sec->text_end directly as the last parameter to
>> mtree_insert_range() break contiguous mappings?
>>
>> mtree_insert_range() expects the last boundary to be inclusive, but
>> sec->text_end represents the exclusive end address of the executable segment.
>> If user space maps seamlessly contiguous text segments, the insertion for the
>> second segment might overlap with the claimed end of the first, causing it to
>> fail with -EEXIST.
> 
> Addressed in previous patch.
> 
>>
>>>   	if (ret) {
>>>   		dbg("mtree_insert_range failed: text=%lx-%lx\n",
>>
> 
> Thanks and regards,
> Jens

Re: [PATCH v14 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Indu Bhagat]

On 2026-05-08 02:49, Jens Remus wrote:
> On 5/6/2026 5:01 PM, Steven Rostedt wrote:
>> On Wed, 6 May 2026 16:34:34 +0200
>> Jens Remus <jremus@linux.ibm.com> wrote:
> 
>>>> If a malicious user provides a large fre_len in the header, fres_end
>>>> (calculated as fres_start + shdr.fre_len) could wrap around the 32-bit
>>>> address space. This would bypass the bounds check in sframe_read_header(),
>>>> allowing fres_start and fdes_start to point into kernel memory. Later, when
>>>> __read_fde() and __find_fre() use unsafe_get_user(), this could lead to
>>>> arbitrary kernel memory disclosure.
>>>
>>> SFrame is currently only supported on 64-bit architectures (i.e. x86-64,
>>> arm64, s390 64-bit).  So unsigned long fres_end should always be 64-bit.
>>> Do we need to add the following to the header parsing?
>>>
>>> if (fdes_start >= fdes_end || fres_start >= fres_end) {
>>> 	dbg_sec("inconsistent FDE/FRE start/end address\n");
>>> 	return -EINVAL;
>>> }
>>
>> I guess this wouldn't hurt.
> 
> Reviewing my suggestion again I realize that this check would be
> superfluous.  The existing computation and check already ensures that
> the FDE table is within sframe section, the FRE table is within sframe
> section, and both tables do not overlap:
> 
> 	num_fdes   = shdr.num_fdes;
> 	fdes_start = header_end + shdr.fdes_off;
> 	fdes_end   = fdes_start + (num_fdes * sizeof(struct sframe_fde_v3));
> 
> 	fres_start = header_end + shdr.fres_off;
> 	fres_end   = fres_start + shdr.fre_len;
> 
> 	if (fres_start < fdes_end || fres_end > sec->sframe_end) {
> 		dbg_sec("inconsistent FDE/FRE offsets\n");
> 		return -EINVAL;
> 	}
> 
> - fdes_start and fres_start are computed from header_start and thus must
>    be larger sframe_start
> - fdes_end and fres_end are computed from their fdes_start and
>    fres_start and thus must be larger than sframe_start
> - fres_start < fdes_end ensures that the FDE table and FRE table do not
>    overlap
> - fres_end > sec->sframe_end ensures that fres_end (and fdes_end and both
>    fdes_start and fres_start) are smaller or equal sframe_end
> 


Yes, I too think the existing check you note above suffices.

Re: [PATCH v14 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Jens Remus]

On 5/6/2026 5:01 PM, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:34:34 +0200
> Jens Remus <jremus@linux.ibm.com> wrote:
> 
>>>> +static __always_inline int __read_fre(struct sframe_section *sec,
>>>> +				      struct sframe_fde_internal *fde,
>>>> +				      unsigned long fre_addr,
>>>> +				      struct sframe_fre_internal *fre)
>>>> +{  
>>> [ ... ]  
>>>> +	if (fre_addr + addr_size + 1 > sec->fres_end)
>>>> +		return -EFAULT;
>>>> +
>>>> +	UNSAFE_GET_USER_INC(ip_off, cur, addr_size, Efault);  
>>>
>>> Will this cause alignment faults on architectures with strict alignment
>>> requirements?
>>>
>>> The .sframe format uses packed structures and variable-length datawords. The
>>> cur pointer might be unaligned here, and UNSAFE_GET_USER_INC() performs
>>> 16-bit or 32-bit reads via unsafe_get_user().  
>>
>> IIUC this should not be an issue for x86-64, s390, and arm64.
> 
> Do we have a way to make sure that sframe support will always be for
> architectures that can handle alignment issues like this? There should
> be something to force this via configs or something that will trigger a
> warning or bug if this is built for architectures that can't handle
> this alignment.

Config option HAVE_EFFICIENT_UNALIGNED_ACCESS seems to be the one that
is used for exactly this purpose.


IIUC config options that get selected should not depend on.  So the
following would not be ok, although it seems to work?

diff --git a/arch/Kconfig b/arch/Kconfig
@@ -495,6 +495,8 @@ config HAVE_UNWIND_USER_FP

 config HAVE_UNWIND_USER_SFRAME
	bool
+	depends on 64BIT
+	depends on HAVE_EFFICIENT_UNALIGNED_ACCESS
	select UNWIND_USER

 config SFRAME_VALIDATION

This would trigger a warning if HAVE_UNWIND_USER_SFRAME is enabled when
64BIT and/or HAVE_EFFICIENT_UNALIGNED_ACCESS are not enabled (disabled
on purpose for testing below):

$ PATH=$HOME/temp/binutils-sframe/bin:$PATH make defconfig W=e
*** Default configuration is based on 'defconfig'

WARNING: unmet direct dependencies detected for HAVE_UNWIND_USER_SFRAME
  Depends on [n]: 64BIT [=y] && HAVE_EFFICIENT_UNALIGNED_ACCESS [=n]
  Selected by [y]:
  - S390 [=y]


Shall I better use BUILD_BUG_ON(!IS_ENABLED(CONFIG_...)) instead?  Would
I add that to all functions that are affected (i.e. __read_fre(),
__read_default_fre_datawords(), and __read_flex_fde_fre_datawords()),
duplicating the build-time error message, or would I better introduce a
dummy function to check this precondition (and 64-bit arch), for
instance:

diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
@@ -886,3 +904,20 @@ void sframe_free_mm(struct mm_struct *mm)

        mtree_destroy(&mm->sframe_mt);
 }
+
+void __init sframe_check(void);
+
+/*
+ * Dummy function to break the build if preconditions not met.
+ */
+void __init sframe_check(void)
+{
+       /* SFrame V3 is only supported on 64-bit architectures */
+       BUILD_BUG_ON(!IS_ENABLED(CONFIG_64BIT));
+
+       /*
+        * Unaligned access to 16/32-bit SFrame FRE fields and datawords
+        * using unsafe_get_user() via UNSAFE_GET_USER_INC()
+        */
+       BUILD_BUG_ON(!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS));
+}

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 05/19] unwind_user/sframe: Add support for reading .sframe contents

[Steven Rostedt]

On Tue, 12 May 2026 15:35:09 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> 
> 
> IIUC config options that get selected should not depend on.  So the
> following would not be ok, although it seems to work?
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> @@ -495,6 +495,8 @@ config HAVE_UNWIND_USER_FP
> 
>  config HAVE_UNWIND_USER_SFRAME
> 	bool
> +	depends on 64BIT
> +	depends on HAVE_EFFICIENT_UNALIGNED_ACCESS

A config selected by architectures should not have architecture
dependencies, as those architectures should not be selecting it!

Archs with both 32 bit and 64 bit should do:

	select HAVE_UNWIND_USER_SFRAME if 64BIT

And no arch without unaligned access should have it selected.

> 	select UNWIND_USER
> 


> 
> Shall I better use BUILD_BUG_ON(!IS_ENABLED(CONFIG_...)) instead?  Would
> I add that to all functions that are affected (i.e. __read_fre(),
> __read_default_fre_datawords(), and __read_flex_fde_fre_datawords()),
> duplicating the build-time error message, or would I better introduce a
> dummy function to check this precondition (and 64-bit arch), for
> instance:

Yeah, that's a better idea. You only need one BUILD_BUG_ON() in the
file for it to have the desired effect. You don't need to add a dummy
function. Just put it in sframe_read_header().

Thanks,

-- Steve