Re: [PATCH v14 11/19] unwind_user/sframe: Show file name in debug output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Potential use-after-free of `sec` and `sec->filename` in the error path of `__sframe_remove_section`.
- [High] A race condition in the `-EFAULT` error path of `sframe_find()` allows an unprivileged local attacker to trigger a `WARN_ON_ONCE` and potentially panic the kernel.
- [High] Unbounded kernel memory allocations controlled by user space do not use `__GFP_ACCOUNT`, allowing memory cgroup limit bypass.
- [Medium] dbg_init() fails to fall back to other VMA naming methods if vma->vm_ops->name(vma) returns NULL.
- [Medium] Directly checking `vma->vm_file` in a conditional to classify the VMA violates VMA architectural rules.
- [Medium] The functions `sframe_find()` and `sframe_add_section()` mix scope-based cleanup helpers with manual `goto`-based cleanup labels, violating kernel refactoring guidelines.
--

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

unwind_user/sframe: Show file name in debug output

This commit adds the file name to debug output messages for sframe sections
to make debugging easier. It records the filename when the section is added
by inspecting the associated VMA.

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> index 8eed6a7d96256..4419626f01734 100644
> --- a/kernel/unwind/sframe.c
> +++ b/kernel/unwind/sframe.c
> @@ -359,14 +359,17 @@ int sframe_find(unsigned long ip, struct unwind_user_frame *frame)
>  end:
>  	user_read_access_end();
>  
> -	if (ret == -EFAULT)
> +	if (ret == -EFAULT) {
> +		dbg_sec("removing bad .sframe section\n");
>  		WARN_ON_ONCE(sframe_remove_section(sec->sframe_start));
> +	}

Can concurrent executions of this error path trigger the WARN_ON_ONCE()? If
two threads hit the EFAULT path concurrently, the first successfully removes
the section, but the second fails to find it and returns EINVAL, which
triggers the warning.

Also, does this code mix goto-based error handling with scope-based cleanup
helpers? The sframe_find() function uses guard(srcu)(&sframe_srcu) while
also using the goto end label. The sframe_add_section() function similarly
mixes scoped_guard with goto. The guidelines specify that functions should
convert all resources to scope-based cleanup or convert none of them.

[ ... ]
> @@ -496,7 +501,7 @@ static int __sframe_remove_section(struct mm_struct *mm,
>  				   struct sframe_section *sec)
>  {
>  	if (!mtree_erase(&mm->sframe_mt, sec->text_start)) {
> -		dbg("mtree_erase failed: text=%lx\n", sec->text_start);
> +		dbg_sec("mtree_erase failed: text=%lx\n", sec->text_start);

Does accessing sec->filename in dbg_sec() risk a use-after-free here? If
mtree_erase() fails, it indicates the section might have been concurrently
removed. The SRCU grace period could expire, causing sec to be freed before
dbg_sec() is called.

[ ... ]
> diff --git a/kernel/unwind/sframe_debug.h b/kernel/unwind/sframe_debug.h
> index 36352124cde81..e568be4172b1b 100644
> --- a/kernel/unwind/sframe_debug.h
> +++ b/kernel/unwind/sframe_debug.h
[ ... ]
> +static inline void dbg_init(struct sframe_section *sec)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +
> +	guard(mmap_read_lock)(mm);
> +	vma = vma_lookup(mm, sec->sframe_start);
> +	if (!vma)
> +		sec->filename = kstrdup("(vma gone???)", GFP_KERNEL);
> +	else if (vma->vm_file)
> +		sec->filename = kstrdup_quotable_file(vma->vm_file, GFP_KERNEL);

Does checking vma->vm_file directly skip proper classification for some
anonymous VMAs? According to the VMA guidelines, certain anonymous VMAs like
private mappings of /dev/zero have vma->vm_file set. Should this use
!vma_is_anonymous(vma) instead?

Do these string allocations need __GFP_ACCOUNT? Since they are triggered by
user space and persist until the section is removed, could a user allocate
large amounts of uncharged memory and bypass memory cgroup limits?

> +	else if (vma->vm_ops && vma->vm_ops->name)
> +		sec->filename = kstrdup(vma->vm_ops->name(vma), GFP_KERNEL);

If vma->vm_ops->name(vma) returns NULL, will kstrdup() return NULL and skip
the subsequent fallback naming methods? Does this leave sec->filename as
NULL, causing debug logs to print (null)?

> +	else if (arch_vma_name(vma))
> +		sec->filename = kstrdup(arch_vma_name(vma), GFP_KERNEL);
> +	else if (!vma->vm_mm)
> +		sec->filename = kstrdup("(vdso)", GFP_KERNEL);
> +	else
> +		sec->filename = kstrdup("(anonymous)", GFP_KERNEL);
> +}

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