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

[Indu Bhagat <ibhagatgnu@gmail.com>]

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