Re: [PATCH v15 12/20] 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:

New issues:
- [High] Missing cond_resched() or fatal signal checking in the nested loops of sframe_validate_section() allows an unprivileged local attacker to trigger a CPU soft lockup.
- [Low] The validation logic in sframe_validate_section() incorrectly rejects a valid .sframe section if the first function is located at virtual address 0.

Pre-existing issues:
- [High] sframe_find() handles read failures by calling sframe_remove_section(), which acquires a lock inside NMI or IRQ context, leading to deadlocks or PREEMPT_RT panics.
--

commit 07d74be031cf9fade2b9ddd98bfdd8f52c38c2da
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Add .sframe validation option

This commit introduces a debugging feature to optionally validate the
entire .sframe section upon loading, rather than checking on-demand.
It uses user_read_access_begin/end to read FDEs and FREs to verify their
formatting and sorting.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
[ ... ]
> int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
> {
[ ... ]
> 	ret = __find_fre(sec, &fde, ip, frame);
> end_uaccess:
> 	user_read_access_end();
> 
> end:
> 	if (ret && ret != -ENOENT) {
> 		dbg_sec("removing bad .sframe section\n");
> 		WARN_ON_ONCE(sframe_remove_section(sec->sframe_start));
> 	}
> 
> 	return ret;
> }

[Severity: High]
This isn't a bug introduced by this patch, but does calling
sframe_remove_section() here risk a deadlock or panic on PREEMPT_RT?

If a page fault occurs during user memory access, unsafe_copy_from_user()
returns -EFAULT to prevent sleeping.

However, sframe_find() handles any error other than -ENOENT by calling
sframe_remove_section(), which in turn calls mtree_erase().

Since mtree_erase() takes an internal spinlock, doing this in NMI context
(where sframe_find() is often called) can deadlock or panic a PREEMPT_RT
kernel, while also permanently deleting a valid unwind section due to
transient memory pressure.

[ ... ]
> +#ifdef CONFIG_SFRAME_VALIDATION
[ ... ]
> +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++) {
> +		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);
> +		if (ret) {
> +			dbg_sec("safe_read_fde(%u) failed\n", i);
> +			return ret;
> +		}
> +
> +		ip = fde.func_addr;
> +		if (ip <= prev_ip) {

[Severity: Low]
Does this check accidentally reject valid sections where the first
function is located at virtual address 0?

Since prev_ip is initialized to 0, validating the first FDE when ip is 0
will cause this check to evaluate to true, falsely triggering an -EINVAL.

Valid executables can map their text section at 0 if vm.mmap_min_addr is
set to 0.

> +			dbg_sec("FDE %u not sorted\n", i);
> +			return -EINVAL;
> +		}
> +		prev_ip = ip;
> +
> +		fre_addr = sec->fres_start + fde.fres_off;
> +		for (j = 0; j < fde.fres_num; j++) {
[ ... ]
> +			fre_addr += fre->size;
> +
> +			if (prev_fre && fre->ip_off <= prev_fre->ip_off) {
> +				dbg_sec("FDE %u: FRE %u not sorted\n", i, j);
> +				return -EINVAL;
> +			}
> +
> +			prev_fre = fre;
> +		}

[Severity: High]
Could a malicious user exploit these loops to trigger a CPU soft lockup?

The variables sec->num_fdes and fde.fres_num are user-controlled and
bounded only by the VMA size.

Without a cond_resched() or fatal signal check, an attacker could supply
large counts and cause these loops to run for seconds without yielding.

On non-preemptible kernels, this monopolizes the CPU and triggers the soft
lockup watchdog, resulting in a local denial of service.

> +	}
> +
> +	return 0;
> +}

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