Re: [PATCH bpf-next 2/3] bpf: Add user memory access kfuncs for linux_binprm

On Mon, Aug 24, 2026 at 3:49 AM Anastasios Papagiannis
<[email protected]> wrote:
>
> Thanks for the review!
>
> > making this linux_binprm-specific seems short-sighted and overly
> > specialized, why not pass `const struct mm *` and call new kfuncs
> > bpf_copy_from_user_mm[_str] ?
>
> > and this seems to belong right next to bpf_copy_from_user_task_str()
> > and others in kernel/bpf/helpers.c (and bpf_copy_from_user_task
> > variants should just delegate to this mm-based APIs and minimize code
> > duplication)
>
> This is exactly what I did in my previous set of patches:
> https://lore.kernel.org/bpf/[email protected]/T/
> https://patchwork.kernel.org/project/netdevbpf/list/?series=1144046&state=*
> https://sashiko.dev/#/patchset/20260811112154.94053-1-tasos.papagiannnis%40gmail.com
> https://github.com/kernel-patches/bpf/pull/13211
>
> where I got AI reviews with 2 main issues:
>
> 1.  Does marking bprm->mm as trusted-or-null here create a use-after-free
>     vulnerability on the execve error path?
>     Looking at free_bprm() in fs/exec.c, when an error occurs:
>     fs/exec.c:free_bprm() {
>         ...
>         if (bprm->mm) {
>             acct_arg_size(bprm, 0);
>             mmput(bprm->mm);
>         }
>         ...
>     }
>     The object is freed via mmput(), but the bprm->mm pointer is not set to NULL.
>     Could subsequent traceable cleanup functions (such as free_arg_pages) expose
>     this dangling reference to BPF programs, bypassing the verifier's safety
>     guarantees?
>
> This is something that I believe is easy to fix.
>
> 2.  Does this break the BPF load-compatibility guarantee documented in
>     Documentation/bpf/bpf_design_QA.rst?
>
>     Before this patch, walking bprm->mm produced a dereferenceable register
>     in every program type. In sleepable programs, in_rcu_cs() is false, so
>     check_ptr_to_btf_access() sets flag = PTR_UNTRUSTED. In non-sleepable
>     programs, in_rcu_cs() is true but mm is not in any __safe_rcu* list, so
>     clear_trusted_flags() produces a plain PTR_TO_BTF_ID.
>
>     Both PTR_TO_BTF_ID and PTR_TO_BTF_ID|PTR_UNTRUSTED satisfy the guard in
>     check_mem_access():
>
>     } else if (base_type(reg->type) == PTR_TO_BTF_ID &&
>                 !type_may_be_null(reg->type)) {
>             err = check_ptr_to_btf_access(env, regs, reg, argno, off, size, t,
>                                             value_regno);
>
>     so bprm->mm->arg_start loads with no NULL check.
>
>     After this patch, btf_nested_type_is_trusted() matches
>     linux_binprm__safe_trusted_or_null.mm, so check_ptr_to_btf_access() sets
>     flag |= PTR_TRUSTED | PTR_MAYBE_NULL. Now type_may_be_null() is true, the
>     arm above is skipped, and control falls to the final else:
>
>     } else {
>             verbose(env, "%s invalid mem access '%s'\n", reg_arg_name(env, argno),
>                     reg_type_str(env, reg->type));
>             return -EACCES;
>     }
>
>     Any existing program that reads through bprm->mm without an explicit NULL
>     check now fails bpf_prog_load() with -EACCES.
>
> This sounds to me a bit more complicated. In a prog attached to lsm/security_bprm_check,
> bprm is trusted but bprm->mm is not trusted. So we need a way to have bprm->mm trusted
> to pass it as an argument to the helpers. In that series, I marked linux_binprm->mm as
> trusted-or-null, which breaks all existing programs that use
> linux_binprm->mm->something.

this is probably ok, but also we might want to look into having
verifier still allow to dereference trusted ptr-or-null into untrusted
ptr_to_btf_id to help with smooth transitions like this.

Not sure how complicated that would be in the verifier, but that would
be a nice backwards compat preserving behavior.

But to keep things simple today, I'd just mark that bprm->mm pointer
as TRUSTED_OR_NULL.

>
> Another approach would be to add one more kfunc (i.e. bpf_get_bprm_mm) to get
> the mm from bprm and have it trusted.

sound quite specialized, but I don't have a strong opinion

>
> The reason that I didn't push that further was that mm in general is
> bound to either linux_binprm or task_struct during its lifetime. We already
> have helpers for task_struct, and I thought that adding helpers for linux_binprm
> would be enough. Other than that, there is only one security_*() function that
> takes mm_struct as an argument (i.e. security_vm_enough_memory_mm), and possibly
> in that case we do not need to read the memory contents, but only metadata
> (possibly I am oversimplifying here, but this is to show my thinking).
>
> Unless I am missing something obvious, I can see two possible approaches here:
>   1. Keep the bprm-specific kfuncs as they are now.

no, I think that's a bad idea

>   2. Rework this around the mm variants that you proposed and add one
>      more kfunc (i.e. bpf_get_bprm_mm) to make that work.
>
> Does the second approach sound reasonable to you?

yes

>
> Thanks,
> Anastasios