Re: [PATCH bpf-next v6 3/5] bpf: Add user memory access kfuncs for mm_struct

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3078 bytes --]

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index b3cc5c8fc875..3338bebdd86e 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -679,9 +679,44 @@ const struct bpf_func_proto bpf_copy_from_user_proto = {
>  	.arg3_type	= ARG_ANYTHING,
>  };
>
> +static int __bpf_copy_from_user_mm(void *dst, u32 size,
> +				   const void __user *user_ptr,
> +				   struct mm_struct *mm)
> +{
> +	int ret;
> +
> +	ret = access_remote_vm(mm, (unsigned long)user_ptr, dst, size, 0);

Does this drop the address-wrap guard on !CONFIG_MMU builds?

The refactor replaces access_process_vm() with access_remote_vm(). On
CONFIG_MMU these are equivalent, but on !CONFIG_MMU they differ.

The old path through access_process_vm() (mm/nommu.c) includes this guard:

  int access_process_vm(struct task_struct *tsk, unsigned long addr,
                        void *buf, int len, unsigned int gup_flags)
  {
          struct mm_struct *mm;

          if (addr + len < addr)      /* wrap guard */
                  return 0;
          mm = get_task_mm(tsk);
          ...
  }

The new path through access_remote_vm() (mm/nommu.c) has no guard:

  int access_remote_vm(struct mm_struct *mm, unsigned long addr,
                       void *buf, int len, unsigned int gup_flags)
  {
          return __access_remote_vm(mm, addr, buf, len, gup_flags);
  }

In __access_remote_vm, the length clamp uses the same wrapped addition:

  vma = find_vma(mm, addr);
  if (vma) {
          if (addr + len >= vma->vm_end)
                  len = vma->vm_end - addr;   /* skipped when wrapped */
          ...
          copy_from_user_page(vma, NULL, addr, buf, (void *) addr, len);

When addr + len wraps to a small value, the check is false, len stays
unclamped, and copy_from_user_page() (a plain memcpy on nommu) reads past
vma->vm_end and past the end of the address space, causing memory
disclosure or a bus fault.

This affects bpf_copy_from_user_task() (which previously had the guard),
and the two new kfuncs bpf_copy_from_user_mm() and
bpf_copy_from_user_mm_str(), since all three call __bpf_copy_from_user_mm().
The address comes directly from the BPF program with no validation on this
path.

The sibling string helper __bpf_copy_from_user_mm_str() does keep the
check via copy_remote_mm_str(), which calls __copy_remote_mm_str() in
mm/nommu.c and uses check_add_overflow(). Only the non-string path lost
it.

This also contradicts the changelog claim "No behavior change is intended
for the existing task-based interfaces" - on !CONFIG_MMU, the behavior of
bpf_copy_from_user_task() does change (a wrapping address previously
returned 0, now reaches the unclamped read).

> +	if (ret == size)
> +		return 0;
> +
> +	memset(dst, 0, size);
> +	/* Return -EFAULT for partial read */
> +	return ret < 0 ? ret : -EFAULT;
> +}

[ ... ]


---
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/34245694915