Re: [PATCH v16 20/20] unwind_user/sframe: Add prctl() interface for registering .sframe sections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] The patch adds a test-only UAPI interface that is explicitly not intended for mainline inclusion, violating ABI stability rules.
- [High] Unbounded memory allocation in PR_ADD_SFRAME allows a local Denial of Service (DoS) via memory exhaustion.
- [Low] When SFrame support is disabled, the prctl options incorrectly return -ENOSYS instead of -EINVAL.

Pre-existing issues:
- [Critical] Improper handling of -EFAULT in sframe_find() leads to premature deletion of valid unwinding metadata and poses a severe NMI deadlock risk.
- [High] A malicious user application can reliably trigger a WARN_ON_ONCE in sframe_find() by causing concurrent removals of an .sframe section, potentially panicking the system.
- [High] Unbounded iteration in sframe_validate_section() without yielding the CPU can cause softlockup DoS.
--

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

unwind_user/sframe: Add prctl() interface for registering .sframe sections

The kernel doesn't have direct visibility to the ELF contents of shared
libraries. This commit adds prctl() interfaces to allow glibc to tell the
kernel where to find .sframe sections for unwinding.

> [
>   This adds an interface for prctl() for testing loading of sframes for
>   libraries. But this interface should really be a system call. This patch
>   is for testing purposes only and should not be applied to mainline.
> ]

[Severity: High]
Since this is explicitly marked as a temporary test patch, should it be
redesigned as a proper system call or removed before being merged?

Adding temporary or experimental prctl() interfaces exposes a temporary ABI to
userspace which must be maintained indefinitely, polluting the UAPI.

> diff --git a/kernel/sys.c b/kernel/sys.c
> index 62e842055cc9c..b0a9b1e3ccd7c 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c

[ ... ]

> @@ -2907,6 +2908,14 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		if (arg3 & PR_CFI_LOCK && !(arg3 & PR_CFI_DISABLE))
>  			error = arch_prctl_lock_branch_landing_pad_state(me);
>  		break;
> +	case PR_ADD_SFRAME:
> +		error = sframe_add_section(arg2, arg3, arg4, arg5);

[Severity: High]
Could this allow a local Denial of Service via memory exhaustion?

A user process can repeatedly call prctl(PR_ADD_SFRAME) with valid, small
ranges. If each call allocates a new struct sframe_section and inserts it
into the mm->sframe_mt maple tree using GFP_KERNEL_ACCOUNT, an unprivileged
user might exhaust kernel memory since there is no upper limit on the total
number of sections a process can register.

[Severity: Low]
Is it intentional that the fallback stub for sframe_add_section() returns
-ENOSYS when SFrame support is disabled?

include/linux/sframe.h:sframe_add_section() {
    ...
    return -ENOSYS;
}

Standard kernel conventions dictate that unsupported or disabled prctl options
should return -EINVAL. Returning -ENOSYS incorrectly indicates to userspace
that the entire prctl syscall is unimplemented.

[Severity: High]
This is a pre-existing issue, but is there a risk of a softlockup Denial of
Service during section validation?

When registering a new .sframe section via PR_ADD_SFRAME, the kernel
synchronously validates all FDE and FRE entries in sframe_validate_section().
Because the number of FDEs is read from user-space memory and can be extremely
large, an attacker could provide a massive amount of FDEs/FREs.

Since the validation loop does not contain cond_resched(), the kernel could
spin here for seconds without yielding, triggering the softlockup detector.

> +		break;
> +	case PR_REMOVE_SFRAME:
> +		if (arg3 || arg4 || arg5)
> +			return -EINVAL;
> +		error = sframe_remove_section(arg2);

[Severity: Critical]
This is a pre-existing issue, but does handling -EFAULT in sframe_find() risk
a severe NMI deadlock and premature deletion of valid metadata?

If unsafe_get_user() cleanly faults because the memory is temporarily paged
out, it sets ret = -EFAULT. The cleanup block then calls
WARN_ON_ONCE(sframe_remove_section(sec->sframe_start)).

Because sframe_find() is used by stack unwinders (like perf) that execute
in NMI context, triggering sframe_remove_section() inside an NMI attempts
to acquire the maple tree's internal spinlock (mt_lock). If the NMI
interrupted a thread already holding mt_lock, this could result in a hard
deadlock. Furthermore, this permanently destroys perfectly valid unwinding
metadata simply because a page was not resident.

[Severity: High]
This is a pre-existing issue, but might a malicious application reliably
trigger the WARN_ON_ONCE in sframe_find(), potentially panicking the system?

If an .sframe section returns -EFAULT because it is paged out, the error
path calls WARN_ON_ONCE(sframe_remove_section(sec->sframe_start)).
If sframe_remove_section() fails to find the section, it returns -EINVAL,
causing the warning to fire.

This condition could be reliably triggered if multiple threads concurrently
unwind through the same paged-out section, or if one thread unwinds while
another explicitly removes the section using the newly added
prctl(PR_REMOVE_SFRAME). If panic_on_warn is enabled, this might allow
unprivileged users to reliably crash the system.

> +		break;
>  	default:
>  		trace_task_prctl_unknown(option, arg2, arg3, arg4, arg5);
>  		error = -EINVAL;

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