Re: [PATCH v15 10/20] unwind_user/sframe: Remove .sframe section on detected corruption

[sashiko-bot@kernel.org]

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

New issues:
- [High] Concurrent stack unwinds encountering an invalid .sframe section will trigger a WARN_ON_ONCE, allowing unprivileged users to cause kernel panics.

Pre-existing issues:
- [Critical] A TOCTOU race in sframe_remove_section leads to SRCU callback list corruption and memory leaks.
--

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

unwind_user/sframe: Remove .sframe section on detected corruption

This commit updates the sframe unwinder to remove an .sframe section
if corruption is detected during the unwind process. It handles access
faults and invalid entries by calling sframe_remove_section() on the
corrupt section.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index f723c1a32f906..02331956009a0 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c

[ ... ]

> @@ -360,16 +360,23 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
>  		return -ENOENT;
>  
>  	if (!user_read_access_begin((void __user *)sec->sframe_start,
> -				    sec->sframe_end - sec->sframe_start))
> -		return -EFAULT;
> +				    sec->sframe_end - sec->sframe_start)) {
> +		ret = -EFAULT;
> +		goto end;
> +	}
>  
>  	ret = __find_fde(sec, ip, &fde);
>  	if (ret)
> -		goto end;
> +		goto end_uaccess;
>  
>  	ret = __find_fre(sec, &fde, ip, frame);
> -end:
> +end_uaccess:
>  	user_read_access_end();
> +
> +end:
> +	if (ret && ret != -ENOENT)
> +		WARN_ON_ONCE(sframe_remove_section(sec->sframe_start));

[Severity: High]
Could this WARN_ON_ONCE be triggered concurrently by unprivileged users?

If multiple threads concurrently encounter an error while unwinding through
an invalid .sframe section, they might both jump to the end label. The
first thread will successfully remove the section and return 0. Subsequent
threads will fail to find the section since it was already removed, causing
sframe_remove_section() to return -EINVAL.

Since -EINVAL is non-zero, this would unconditionally trigger the
WARN_ON_ONCE. If panic_on_warn is enabled, could this allow a malformed
section to intentionally crash the system?

[Severity: Critical]
This is a pre-existing issue, but does sframe_remove_section() have a race
condition that can corrupt the srcu callback list?

Looking at sframe_remove_section(), it locklessly iterates mm->sframe_mt
using mt_for_each() to find the section pointer:

kernel/unwind/sframe.c:sframe_remove_section() {
    ...
    mt_for_each(&mm->sframe_mt, sec, index, ULONG_MAX) {
        if (sec->sframe_start == sframe_start) {
            found = true;
            ret |= __sframe_remove_section(mm, sec);
        }
    }
    ...
}

If a concurrent thread erases the original section and inserts a new one
at the same text_start, mtree_erase() inside __sframe_remove_section()
would remove the new section. The code would then unconditionally call
call_srcu() with the old sec pointer.

Because the old section is already pending in the srcu callback list,
would double-queuing it corrupt the list and permanently leak the newly
erased section?

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