Re: [PATCH v15 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 3 potential issue(s) to consider:
- [High] The patch introduces a temporary testing UAPI into `prctl` which is explicitly noted by the author as not intended for mainline.
- [High] Concurrent prctl(PR_ADD_SFRAME) and prctl(PR_REMOVE_SFRAME) calls lack synchronization, leading to use-after-free, double-free, and memory leaks.
- [Medium] Unsupported prctl options PR_ADD_SFRAME and PR_REMOVE_SFRAME incorrectly return -ENOSYS instead of -EINVAL when the sframe feature is disabled.
--

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

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

This commit adds new prctl interfaces to allow glibc to inform the kernel
where to find .sframe sections. This provides the kernel with visibility
into the ELF contents of shared libraries.

>     [
>       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]
Should this patch be merged if it creates a permanent UAPI via prctl for a
temporary testing feature? 

The commit message explicitly notes it is for testing purposes only and
should not be applied to mainline. Committing to this interface could result
in userspace relying on a temporary test mechanism.

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

[Severity: High]
Are concurrent calls to PR_ADD_SFRAME and PR_REMOVE_SFRAME properly
synchronized to prevent double-free or memory leaks?

If sframe_remove_section() only holds an SRCU read lock and
sframe_add_section() only holds mmap_read_lock(), could Thread A and Thread
B concurrently executing PR_REMOVE_SFRAME both find the same section and
erase it?

If Thread A calls mtree_erase() and queues the section for freeing via
call_srcu(), and Thread C concurrently inserts a new section at the same
text_start via PR_ADD_SFRAME:

Thread B might then erase the new section but queue the old section's RCU
callback a second time. Could this sequence lead to a double-free of the
old section and a memory leak of the newly inserted section?

[Severity: Medium]
When the sframe feature is disabled in the kernel configuration, will these
calls return -ENOSYS instead of -EINVAL?

The dummy functions defined in include/linux/sframe.h return -ENOSYS:

static inline int sframe_add_section(unsigned long sframe_start,
                                     unsigned long sframe_end,
                                     unsigned long text_start,
                                     unsigned long text_end)
{
        return -ENOSYS;
}

According to the prctl ABI convention, unsupported options typically fall
through to the default case and return -EINVAL. Could returning -ENOSYS
cause unexpected errors for userspace applications probing for this feature's
availability?

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