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

[Jens Remus <jremus@linux.ibm.com>]

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/