Re: [PATCH v15 05/20] unwind_user/sframe: Add support for reading .sframe contents

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect FRE size calculation due to using a mutated `dataword_count`.
- [High] Binary search incorrectly reads 64 bits for a 32-bit PC-relative offset, corrupting the function address calculation.
- [High] Unbounded linear scan of user-controlled SFrame FREs can cause a kernel hard lockup.
- [High] Usage of `guard(srcu)` invokes `srcu_read_lock()`, which is not NMI-safe and can corrupt the SRCU lock counter.
- [Medium] Mixing goto-based error handling with scope-based `guard()` cleanup.
--

commit 12c453957aeaea0a5733d7ed09d97cf7dfd380a5
Author: Josh Poimboeuf <jpoimboe@kernel.org>

unwind_user/sframe: Add support for reading .sframe contents

In preparation for using sframe to unwind user space stacks, add an
sframe_find() interface for finding the sframe information associated
with a given text address.

Link: https://lore.kernel.org/all/77c0d1ec143bf2a53d66c4ecb190e7e0a576fbfd.1737511963.git.jpoimboe@kernel.org/
Link: https://lore.kernel.org/all/b35ca3a3-8de5-4d32-8d30-d4e562f6b0de@linux.ibm.com/

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index 6b3ce3f8966d0..a38f50a363633 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c

[ ... ]

> @@ -15,9 +15,350 @@
[ ... ]
> +static __always_inline int __find_fde(struct sframe_section *sec,
> +				      unsigned long ip,
> +				      struct sframe_fde_internal *fde)
> +{
[ ... ]
> +	while (low <= high) {
> +		struct sframe_fde_v3 __user *mid;
> +		s64 func_off;
> +		unsigned long func_addr;
> +
> +		mid = low + ((high - low) / 2);
> +
> +		unsafe_get_user(func_off, (s64 __user *)mid, Efault);

[Severity: High]
Does this read 64 bits for a 32-bit PC-relative offset? Since SFrame V3
specifies a 32-bit signed integer for the PC-relative offset followed by
a 32-bit function size, won't reading 8 bytes merge both fields into
func_off and corrupt the function address calculation?

> +		func_addr = (unsigned long)mid + func_off;

[ ... ]

> +static __always_inline int __read_fre(struct sframe_section *sec,
> +				      struct sframe_fde_internal *fde,
> +				      unsigned long fre_addr,
> +				      struct sframe_fre_internal *fre)
> +{
[ ... ]
> +	UNSAFE_GET_USER_INC(cfa_off, cur, dataword_size, Efault);
> +	dataword_count--;
> +
> +	ra_off = sec->ra_off;
> +	if (!ra_off) {
> +		if (!dataword_count--)
> +			return -EINVAL;
> +
> +		UNSAFE_GET_USER_INC(ra_off, cur, dataword_size, Efault);
> +	}
> +
> +	fp_off = sec->fp_off;
> +	if (!fp_off && dataword_count) {
> +		dataword_count--;
> +		UNSAFE_GET_USER_INC(fp_off, cur, dataword_size, Efault);
> +	}
> +
> +	if (dataword_count)
> +		return -EINVAL;
> +
> +	fre->size	= addr_size + 1 + (dataword_count * dataword_size);

[Severity: High]
Is the FRE size calculation correct here? Since dataword_count is decremented
as datawords are read, and the previous line ensures it is exactly 0, doesn't
this entirely omit the size of the read datawords from the total FRE size?
When __find_fre() advances to the next FRE, this might cause it to point to
the middle of the current FRE's datawords and read garbage.

> +	fre->ip_off	= ip_off;

[ ... ]

> +static __always_inline int __find_fre(struct sframe_section *sec,
> +				      struct sframe_fde_internal *fde,
> +				      unsigned long ip,
> +				      struct unwind_user_frame *frame)
> +{
[ ... ]
> +	fre_addr = sec->fres_start + fde->fres_off;
> +
> +	for (i = 0; i < fde->fres_num; i++) {
> +		int ret;

[Severity: High]
Could this loop cause a kernel hard lockup? fde->fres_num is read from user
space and can be large. Since this performs a linear scan reading each FRE
sequentially from user memory, and unwinding often runs in NMI context (like
perf PMU interrupts), could a maliciously large SFrame section force the
kernel to loop without yielding and trigger the NMI watchdog?

> +
> +		/*
> +		 * Alternate between the two fre_addr[] entries for 'fre' and
> +		 * 'prev_fre'.
> +		 */

[ ... ]

> +int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct sframe_section *sec;
> +	struct sframe_fde_internal fde;
> +	int ret;
> +
> +	if (!mm)
> +		return -EINVAL;
> +
> +	guard(srcu)(&sframe_srcu);

[Severity: High]
Is this safe to use from NMI context? Stack unwinding is frequently invoked
from NMIs (like perf PMU interrupts), but srcu_read_lock() is not NMI-safe
on all architectures (like arm64, where the non-atomic read-modify-write
could corrupt the SRCU lock counter). Should this use srcu_read_lock_nmisafe()
or standard RCU instead?

[Severity: Medium]
Does this mix goto-based error handling with scope-based cleanup? The
function declares a scope-based guard here, but uses a goto-based cleanup
label (end:) below. This goes against the cleanup subsystem guidelines
which recommend either converting all resources to scope-based cleanup or
none at all, to avoid confusing ownership semantics.

> +
> +	sec = mtree_load(&mm->sframe_mt, ip);
> +	if (!sec)
> +		return -ENOENT;
> +
> +	if (!user_read_access_begin((void __user *)sec->sframe_start,
> +				    sec->sframe_end - sec->sframe_start))
> +		return -EFAULT;
> +
> +	ret = __find_fde(sec, ip, &fde);
> +	if (ret)
> +		goto end;
> +
> +	ret = __find_fre(sec, &fde, ip, frame);
> +end:
> +	user_read_access_end();
> +	return ret;
> +}

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