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

[Indu Bhagat <ibhagatgnu@gmail.com>]

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.