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

[Steven Rostedt <rostedt@kernel.org>]

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