Re: [PATCH bpf-next v3 2/7] bpf: Add user memory access kfuncs for mm_struct

[bot+bpf-ci]

> bpf: Add user memory access kfuncs for mm_struct
>
> On CONFIG_MMU kernels, when security_bprm_check() runs, the argument and
> environment strings for the exec have been copied into bprm->mm. The new
> address space is not associated with a task_struct until exec_mmap(), so
> existing BPF user memory helpers can only read from the calling task's old
> address space.
>
> Add bpf_copy_from_user_mm() and bpf_copy_from_user_mm_str() kfuncs. Both
> take a struct mm_struct pointer directly, allowing callers to access
> trusted address spaces that are not associated with a task_struct.
>
> bpf_copy_from_user_mm() has similar semantics to
> bpf_copy_from_user_task(). bpf_copy_from_user_mm_str() copies one
> NUL-terminated string and returns its size including the NUL terminator.
> It accepts BPF_F_PAD_ZEROS to clear unused destination bytes on success.
>
> Register both kfuncs and mark them KF_SLEEPABLE because accessing the
> remote address space can fault.
>
> On !CONFIG_MMU, exec argument and environment strings remain in
> bprm->page[] until the binary loader transfers them to the new process
> stack. They are therefore not accessible through bprm->mm at the
> bprm_check_security hook. The linux_binprm use described above is
> CONFIG_MMU-only, although the new kfuncs remain available for other
> address spaces on !CONFIG_MMU.

This isn't a bug, but the changelog describes the patch as adding two
new kfuncs, without noting that it also rewrites two existing entry
points. BPF_CALL_5(bpf_copy_from_user_task) becomes a get_task_mm()
wrapper around bpf_copy_from_user_mm() instead of calling
access_process_vm() directly, and bpf_copy_from_user_task_str()
becomes a wrapper around bpf_copy_from_user_mm_str() instead of
calling copy_remote_vm_str().

Could the changelog note that bpf_copy_from_user_task() and
bpf_copy_from_user_task_str() are refactored into wrappers with no
intended behaviour change?

Also, the closing paragraph describes !CONFIG_MMU bprm->page[]
behaviour that this patch does not implement or depend on. Would that
paragraph sit better in the selftest patch that acts on it?

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b3cc5c8fc8756..19c01aa734ae1 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -30,10 +30,17 @@
>  #include <linux/irq_work.h>
>  #include <linux/buildid.h>
>
>  #include "../../lib/kstrtox.h"
>
> +int bpf_copy_from_user_mm(void *dst, u32 dst__sz,
> +			  const void __user *unsafe_ptr__ign,
> +			  struct mm_struct *mm, u64 flags);
> +int bpf_copy_from_user_mm_str(void *dst, u32 dst__sz,
> +			      const void __user *unsafe_ptr__ign,
> +			      struct mm_struct *mm, u64 flags);
> +

This isn't a bug, but these are the only function forward declarations
in kernel/bpf/helpers.c. The first is unavoidable: bpf_copy_from_user_mm()
is called from BPF_CALL_5(bpf_copy_from_user_task) about 650 lines above
its definition. The second is not: bpf_copy_from_user_mm_str() has exactly
one in-file caller, bpf_copy_from_user_task_str(), and the new code defines
the wrapper before the function it wraps.

Could bpf_copy_from_user_task_str() move below bpf_copy_from_user_mm_str()
so only one forward declaration is needed?

Also, both prototypes omit __bpf_kfunc and sit outside the
__bpf_kfunc_start_defs()/__bpf_kfunc_end_defs() region, while the
definitions add __used/__retain/noinline through the macro. Every other
kfunc in the tree is declared and defined with __bpf_kfunc.

Should the remaining prototype carry __bpf_kfunc to match the definition?

>  /* If kernel subsystem is allowing eBPF programs to call this function,
>   * inside its own verifier_ops->get_func_proto() callback it should return
>   * bpf_map_lookup_elem_proto, so that verifier can properly check the arguments

[ ... ]

> @@ -680,26 +687,19 @@ const struct bpf_func_proto bpf_copy_from_user_proto = {
>  };
>
>  BPF_CALL_5(bpf_copy_from_user_task, void *, dst, u32, size,
>  	   const void __user *, user_ptr, struct task_struct *, tsk, u64, flags)
>  {
> +	struct mm_struct *mm;
>  	int ret;
>
> -	/* flags is not used yet */
> -	if (unlikely(flags))
> -		return -EINVAL;
> -
> -	if (unlikely(!size))
> -		return 0;
> -
> -	ret = access_process_vm(tsk, (unsigned long)user_ptr, dst, size, 0);
> -	if (ret == size)
> -		return 0;
> +	mm = get_task_mm(tsk);
> +	ret = bpf_copy_from_user_mm(dst, size, user_ptr, mm, flags);
> +	if (mm)
> +		mmput(mm);
>
> -	memset(dst, 0, size);
> -	/* Return -EFAULT for partial read */
> -	return ret < 0 ? ret : -EFAULT;
> +	return ret;
>  }

[ ... ]

> @@ -3678,20 +3678,105 @@ __bpf_kfunc int bpf_copy_from_user_str(void *dst, u32 dst__sz, const void __user
>   * A negative error code on failure.
>   */
>  __bpf_kfunc int bpf_copy_from_user_task_str(void *dst, u32 dst__sz,
>  					    const void __user *unsafe_ptr__ign,
>  					    struct task_struct *tsk, u64 flags)
> +{
> +	struct mm_struct *mm;
> +	int ret;
> +
> +	mm = get_task_mm(tsk);
> +	ret = bpf_copy_from_user_mm_str(dst, dst__sz, unsafe_ptr__ign,
> +					mm, flags);
> +	if (mm)
> +		mmput(mm);
> +
> +	return ret;
> +}
> +
> +/**
> + * bpf_copy_from_user_mm() - Copy data from an address space
> + * @dst:             Destination address, in kernel space
> + * @dst__sz:         Number of bytes to copy
> + * @unsafe_ptr__ign: Source address in the address space
> + * @mm:              Address space to copy from
> + * @flags:           Reserved for future use; must be zero
> + *
> + * Copies data from the user address space associated with @mm. The destination
> + * is zeroed if an attempted copy cannot be completed in full. Unsupported
> + * flags return -EINVAL without modifying @dst.
> + *
> + * Return: 0 on success, -EINVAL if @flags is non-zero, or -EFAULT if the copy
> + * fails or is partial.
> + */
> +__bpf_kfunc int bpf_copy_from_user_mm(void *dst, u32 dst__sz,
> +				      const void __user *unsafe_ptr__ign,
> +				      struct mm_struct *mm, u64 flags)
> +{
> +	int ret;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (unlikely(!dst__sz))
> +		return 0;
> +
> +	if (unlikely(!mm)) {
> +		memset(dst, 0, dst__sz);
> +		return -EFAULT;
> +	}
> +
> +	ret = access_remote_vm(mm, (unsigned long)unsafe_ptr__ign,
> +			       dst, dst__sz, 0);
> +	if (ret == dst__sz)
> +		return 0;
> +
> +	memset(dst, 0, dst__sz);
> +	return ret < 0 ? ret : -EFAULT;
> +}
> +
> +/**
> + * bpf_copy_from_user_mm_str() - Copy a string from an address space
> + * @dst:             Destination address, in kernel space. This buffer must be
> + *                   at least @dst__sz bytes long
> + * @dst__sz:         Maximum number of bytes to copy, including the trailing NUL
> + * @unsafe_ptr__ign: Source address in the address space
> + * @mm:              Address space to copy from
> + * @flags:           The only supported flag is BPF_F_PAD_ZEROS
> + *
> + * Copies a NUL-terminated string from the user address space associated with
> + * @mm. If the string is too long, @dst is still NUL-terminated unless @dst__sz
> + * is zero.
> + *
> + * If the flags are valid and BPF_F_PAD_ZEROS is set, the unused portion of
> + * @dst is cleared on success and all of @dst is cleared on a copy failure.
> + * Unsupported flags return -EINVAL without modifying @dst.
> + *
> + * Return: The number of copied bytes including the NUL terminator on success,
> + * or a negative error code on failure.
> + */
> +__bpf_kfunc int bpf_copy_from_user_mm_str(void *dst, u32 dst__sz,
> +					  const void __user *unsafe_ptr__ign,
> +					  struct mm_struct *mm, u64 flags)
>  {
>  	int ret;
>
>  	if (unlikely(flags & ~BPF_F_PAD_ZEROS))
>  		return -EINVAL;
>
>  	if (unlikely(dst__sz == 0))
>  		return 0;
>
> -	ret = copy_remote_vm_str(tsk, (unsigned long)unsafe_ptr__ign, dst, dst__sz, 0);
> +	if (unlikely(!mm)) {
> +		if (flags & BPF_F_PAD_ZEROS)
> +			memset(dst, 0, dst__sz);
> +		else
> +			*(char *)dst = '\0';
> +		return -EFAULT;
> +	}
> +
> +	ret = copy_remote_mm_str(mm, (unsigned long)unsafe_ptr__ign, dst, dst__sz, 0);
>  	if (ret < 0) {
>  		if (flags & BPF_F_PAD_ZEROS)
>  			memset(dst, 0, dst__sz);
>  		return ret;
>  	}

[ ... ]

> @@ -4922,10 +5007,12 @@ BTF_ID_FLAGS(func, bpf_preempt_disable)
>  BTF_ID_FLAGS(func, bpf_preempt_enable)
>  BTF_ID_FLAGS(func, bpf_iter_bits_new, KF_ITER_NEW)
>  BTF_ID_FLAGS(func, bpf_iter_bits_next, KF_ITER_NEXT | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_bits_destroy, KF_ITER_DESTROY)
>  BTF_ID_FLAGS(func, bpf_copy_from_user_str, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_mm, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_mm_str, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_copy_from_user_task_str, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_get_kmem_cache)
>  BTF_ID_FLAGS(func, bpf_iter_kmem_cache_new, KF_ITER_NEW | KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_iter_kmem_cache_next, KF_ITER_NEXT | KF_RET_NULL | KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE)


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/33379004067