Re: [PATCH v14 12/19] unwind_user/sframe: Add .sframe validation option

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Denial of Service (Soft Lockup) due to unyielding loops in .sframe section validation.
- [High] Integer overflows on 32-bit architectures bypass bounds checks, allowing arbitrary kernel/MMIO reads.
- [Low] Incorrect format specifier '%d' used for unsigned 32-bit integer.
--

commit 1f173c4dc22a264948bc63183c96747aa7022119
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Add .sframe validation option

This commit adds a debug feature to validate all .sframe sections when
first loading the file rather than on demand. 

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index 4419626f01734..45988cdc5c37a 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c

[ ... ]

> +static int sframe_validate_section(struct sframe_section *sec)
> +{
> +	unsigned long prev_ip = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < sec->num_fdes; i++) {

Can a maliciously crafted .sframe mapping with extremely large values for
sec->num_fdes and fde.fres_num cause a soft lockup?

The outer and inner loops iterate over values parsed directly from the
user-provided .sframe section header, which could be up to 4 billion.

Since there is no cond_resched() called within these loops, processing a
large section might monopolize the CPU for an extended period and trigger
the soft lockup watchdog. 

> +		struct sframe_fre_internal *fre, *prev_fre = NULL;
> +		unsigned long ip, fre_addr;
> +		struct sframe_fde_internal fde;
> +		struct sframe_fre_internal fres[2];
> +		bool which = false;
> +		unsigned int j;
> +		int ret;
> +
> +		ret = safe_read_fde(sec, i, &fde);

[ ... ]

> +		fre_addr = sec->fres_start + fde.fres_off;
> +		for (j = 0; j < fde.fres_num; j++) {
> +			int ret;
> +
> +			fre = which ? fres : fres + 1;
> +			which = !which;
> +
> +			ret = safe_read_fre(sec, &fde, fre_addr, fre);
> +			if (ret) {
> +				dbg_sec("FDE %u: safe_read_fre(%u) failed\n", i, j);
> +				dbg_sec("FDE: func_addr:%#lx func_size:%#x fda_off:%#x fres_off:%#x fres_num:%d info:%u info2:%u rep_size:%u\n",

This isn't a bug, but does fres_num use the wrong format specifier?

The fres_num field is defined as a u32, so %u should probably be used
instead of %d. If a user provides a fres_num exceeding 0x7FFFFFFF, it will
incorrectly print as a negative number in the debug output.

> +					fde.func_addr, fde.func_size,
> +					fde.fda_off,
> +					fde.fres_off, fde.fres_num,
> +					fde.info, fde.info2,
> +					fde.rep_size);
> +				return ret;
> +			}
> +
> +			fre_addr += fre->size;

Could this addition overflow on 32-bit architectures?

Calculations like fre_addr += fre->size (and similarly
fda_addr = sec->fres_start + _fde.fres_off in __read_fde() called by
safe_read_fde()) use 32-bit arithmetic on 32-bit architectures.

A maliciously crafted .sframe section could cause these calculations to wrap
around to a small value, which perfectly bypasses the bounds checks.

Because user_read_access_begin() validates the whole section range upfront
and UNSAFE_GET_USER_INC omits per-access access_ok() checks, this wrap
around could allow arbitrary kernel or MMIO reads.

> +
> +			if (prev_fre && fre->ip_off <= prev_fre->ip_off) {
> +				dbg_sec("FDE %u: FRE %u not sorted\n", i, j);
> +				return -EFAULT;
> +			}
> +
> +			prev_fre = fre;
> +		}
> +	}
> +
> +	return 0;
> +}

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