Re: [PATCH v14 16/19] unwind_user/sframe: Add support for SFrame V3 flexible FDEs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Valid flexible FREs ending in a single padding data word are incorrectly rejected.
- [High] Valid flexible FRE rules for CFA and FP/RA produce unhandled enums, triggering user-controlled WARN_ON_ONCE().
- [High] Outermost frame unwinding is unconditionally broken, causing unintended fallback to frame pointers.
--

commit 7d3289b2e756d08c38877458400ce276ff3f5994
Author: Jens Remus <jremus@linux.ibm.com>

unwind_user/sframe: Add support for SFrame V3 flexible FDEs

SFrame V3 introduces flexible FDEs in addition to the regular FDEs.
The key difference is that flexible FDEs encode the CFA, RA, and FP
tracking information using two FRE data words, a control word and an
offset, or a single padding data word of zero.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index 2721f4af53fd3..2ba2c8b385f96 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> @@ -189,16 +193,147 @@ static __always_inline int __find_fde(struct sframe_section *sec,
[ ... ]
> +static __always_inline int
> +__read_flex_fde_fre_datawords(struct sframe_section *sec,
> +			      struct sframe_fde_internal *fde,
> +			      unsigned long cur,
> +			      unsigned char dataword_count,
> +			      unsigned char dataword_size,
> +			      struct sframe_fre_internal *fre)
> +{
> +	u32 cfa_ctl, ra_ctl, fp_ctl;
> +	s32 cfa_off, ra_off, fp_off;
> +
> +	if (dataword_count < 2)
> +		return -EFAULT;
> +	UNSAFE_GET_USER_INC(cfa_ctl, cur, dataword_size, Efault);
> +	UNSAFE_GET_USER_INC(cfa_off, cur, dataword_size, Efault);
> +	dataword_count -= 2;
> +
> +	ra_off = sec->ra_off;
> +	ra_ctl = ra_off ? 2 : 0; /* regnum=0, deref_p=(ra_off != 0), reg_p=0 */
> +	if (dataword_count >= 2) {

The commit message mentions that flexible FDEs can use a single padding
data word of zero. However, this code checks dataword_count >= 2 for both
the RA and FP blocks.

If an FRE ends with a single padding data word, it will bypass both of these
blocks and reach the check below:

> +		UNSAFE_GET_USER_INC(ra_ctl, cur, dataword_size, Efault);
> +		dataword_count--;
> +		if (ra_ctl) {
> +			UNSAFE_GET_USER_INC(ra_off, cur, dataword_size, Efault);
> +			dataword_count--;
> +		} else {
> +			/* Padding RA location info */
> +			ra_ctl = ra_off ? 2 : 0; /* re-deduce (see above) */
> +		}
> +	}
> +
> +	fp_off = sec->fp_off;
> +	fp_ctl = fp_off ? 2 : 0; /* regnum=0, deref_p=(fp_off != 0), reg_p=0 */
> +	if (dataword_count >= 2) {
> +		UNSAFE_GET_USER_INC(fp_ctl, cur, dataword_size, Efault);
> +		dataword_count--;
> +		if (fp_ctl) {
> +			UNSAFE_GET_USER_INC(fp_off, cur, dataword_size, Efault);
> +			dataword_count--;
> +		} else {
> +			/* Padding FP location info */
> +			fp_ctl = fp_off ? 2 : 0; /* re-deduce (see above) */
> +		}
> +	}
> +
> +	if (dataword_count)
> +		return -EFAULT;

Could this incorrectly reject valid FREs ending in a single padding data word
since dataword_count would be 1 here?

> +
> +	fre->cfa_ctl	= cfa_ctl;
> +	fre->cfa_off	= cfa_off;
> +	fre->ra_ctl	= ra_ctl;
> +	fre->ra_off	= ra_off;
> +	fre->fp_ctl	= fp_ctl;
> +	fre->fp_off	= fp_off;
> +
> +	return 0;
> +
> +Efault:
> +	return -EFAULT;
> +}
[ ... ]
>  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 (!dataword_count) {
>  		/*
>  		 * A FRE without data words indicates RA undefined /
>  		 * outermost frame.
>  		 */
> -		cfa_off	= 0;
> -		ra_off	= 0;
> -		fp_off	= 0;
> -		goto done;
> -	}
> +		fre->cfa_ctl	= 0;
> +		fre->cfa_off	= 0;
> +		fre->ra_ctl	= 0;
> +		fre->ra_off	= 0;
> +		fre->fp_ctl	= 0;
> +		fre->fp_off	= 0;
>  
> -	UNSAFE_GET_USER_INC(cfa_off, cur, dataword_size, Efault);

When __read_fre() processes an outermost frame, it zeroes out fre->cfa_ctl.
In __find_fre(), this is later passed to sframe_init_cfa_rule_data():

> -static __always_inline void
> +static __always_inline int
>  sframe_init_cfa_rule_data(struct unwind_user_cfa_rule_data *cfa_rule_data,
> -			  unsigned char fre_info,
> -			  s32 offset)
> +			  u32 ctlword, s32 offset)
>  {
> -	if (SFRAME_V3_FRE_CFA_BASE_REG_ID(fre_info) == SFRAME_BASE_REG_FP)
> -		cfa_rule_data->rule = UNWIND_USER_CFA_RULE_FP_OFFSET;
> -	else
> -		cfa_rule_data->rule = UNWIND_USER_CFA_RULE_SP_OFFSET;
> +	bool deref_p = SFRAME_V3_FLEX_FDE_CTRLWORD_DEREF_P(ctlword);
> +	bool reg_p = SFRAME_V3_FLEX_FDE_CTRLWORD_REG_P(ctlword);
> +
> +	if (reg_p) {
> +		unsigned int regnum = SFRAME_V3_FLEX_FDE_CTRLWORD_REGNUM(ctlword);
> +
> +		switch (regnum) {
> +		case SFRAME_REG_SP:
> +			cfa_rule_data->rule = UNWIND_USER_CFA_RULE_SP_OFFSET;
> +			break;
> +		case SFRAME_REG_FP:
> +			cfa_rule_data->rule = UNWIND_USER_CFA_RULE_FP_OFFSET;
> +			break;
> +		default:
> +			cfa_rule_data->rule = UNWIND_USER_CFA_RULE_REG_OFFSET;
> +			cfa_rule_data->regnum = regnum;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}

Because ctlword is 0 for an outermost frame, reg_p will evaluate to false,
and this unconditionally returns -EINVAL.

Does this unintentionally break outermost frame unwinding? Returning an
error here will cause __find_fre() to fail before it can reach the
frame->outermost check, resulting in a fallback to frame pointer unwinding
instead of stopping gracefully.

> +
> +	if (deref_p)
> +		cfa_rule_data->rule |= UNWIND_USER_RULE_DEREF;
> +

Also, does this append UNWIND_USER_RULE_DEREF to CFA rules in a way that is
not supported by the unwinder?

If a CFA rule uses SP/FP with deref_p=1, this creates a combined enum like
UNWIND_USER_CFA_RULE_SP_OFFSET | UNWIND_USER_RULE_DEREF. Because .sframe
sections are user-supplied, could an attacker use this to trigger an
unhandled enum path and hit a WARN_ON_ONCE() during unwinding?

>  	cfa_rule_data->offset = offset;
> +
> +	return 0;
>  }
>  
>  static __always_inline void
>  sframe_init_rule_data(struct unwind_user_rule_data *rule_data,
> -		      s32 offset)
> +		      u32 ctlword, s32 offset)
>  {
> -	if (offset) {
> -		rule_data->rule = UNWIND_USER_RULE_CFA_OFFSET_DEREF;
> -		rule_data->offset = offset;
> -	} else {
> +	bool deref_p = SFRAME_V3_FLEX_FDE_CTRLWORD_DEREF_P(ctlword);
> +	bool reg_p = SFRAME_V3_FLEX_FDE_CTRLWORD_REG_P(ctlword);
> +
> +	if (!ctlword && !offset) {
>  		rule_data->rule = UNWIND_USER_RULE_RETAIN;
> +		return;
> +	}
> +	if (reg_p) {
> +		unsigned int regnum = SFRAME_V3_FLEX_FDE_CTRLWORD_REGNUM(ctlword);
> +
> +		rule_data->rule = UNWIND_USER_RULE_REG_OFFSET;
> +		rule_data->regnum = regnum;
> +	} else {
> +		rule_data->rule = UNWIND_USER_RULE_CFA_OFFSET;
>  	}
> +
> +	if (deref_p)
> +		rule_data->rule |= UNWIND_USER_RULE_DEREF;
> +
> +	rule_data->offset = offset;
>  }

Similarly, if a rule for FP/RA is set to *(CFA + offset) with reg_p=0
and deref_p=0, this assigns UNWIND_USER_RULE_CFA_OFFSET without the
UNWIND_USER_RULE_DEREF flag.

Is this combination handled by the rest of the unwinding logic, or will
it fall through to an unsupported state and abort unwinding?

>  
>  static __always_inline int __find_fre(struct sframe_section *sec,
> @@ -344,9 +492,10 @@ static __always_inline int __find_fre(struct sframe_section *sec,
>  		return -EINVAL;
>  	fre = prev_fre;
>  
> -	sframe_init_cfa_rule_data(&frame->cfa, fre->info, fre->cfa_off);
> -	sframe_init_rule_data(&frame->ra, fre->ra_off);
> -	sframe_init_rule_data(&frame->fp, fre->fp_off);
> +	if (sframe_init_cfa_rule_data(&frame->cfa, fre->cfa_ctl, fre->cfa_off))
> +		return -EINVAL;
> +	sframe_init_rule_data(&frame->ra, fre->ra_ctl, fre->ra_off);
> +	sframe_init_rule_data(&frame->fp, fre->fp_ctl, fre->fp_off);
>  	frame->outermost = SFRAME_V3_FRE_RA_UNDEFINED_P(fre->info);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505121718.3572346-1-jremus@linux.ibm.com?part=16