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

On 5/6/2026 5:01 PM, Steven Rostedt wrote:
> On Wed, 6 May 2026 16:34:34 +0200
> Jens Remus <[email protected]> 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)
[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/