Re: [RFC PATCH 06/10] uaccess: Refuse and warn on user copies when current->mm is NULL

[bot+bpf-ci@kernel.org]

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

> uaccess: Refuse and warn on user copies when current->mm is NULL

TL;DR:
- Possible spurious -EFAULT on valid kernel addresses on xtensa/nios2 from
  mm-less tasks (kworkers), e.g. d_path
- nofault helpers may hit the new WARN_ON_ONCE (BPF/tracing); fix is later in
  series, breaking bisection

> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index eddbbb65ccc4..e54a6d804bf9 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -50,6 +50,23 @@

[ ... ]

> +static __always_inline bool uaccess_should_fail(void)
> +{
> +	if (WARN_ON_ONCE(!current->mm))
> +		return true;
> +	return should_fail_usercopy();
> +}

[ ... ]

> @@ -114,7 +131,7 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
>  
>  	might_fault();
>  	instrument_copy_from_user_before(to, from, n);
> -	if (should_fail_usercopy())
> +	if (uaccess_should_fail())
>  		return n;
>  	check_object_size(to, n, false);
>  	res = raw_copy_from_user(to, from, n);

Can this refuse legitimate kernel address copies on xtensa and nios2?

On both architectures __copy_from_user() and __copy_to_user() also sit under
the generic kernel nofault accessors.  Neither architecture defines
__get_kernel_nofault or arch_get_kernel_nofault, so both use the fallback in
include/linux/uaccess.h, which goes through __get_user()/__put_user() on the
kernel address:

    #define __get_kernel_nofault(dst, src, type, label)    \
    ...
            if (__get_user(data, p))                       \
                    goto label;                            \

On xtensa, 8-byte __get_user()/__put_user() are implemented with
__copy_from_user()/__copy_to_user() (arch/xtensa/include/asm/uaccess.h):

    case 8: {
            u64 __x;
            if (unlikely(__copy_from_user(&__x, ptr, 8))) {
    ...
            retval = __copy_to_user(ptr, &__v64, 8) ? -EFAULT : 0;

On nios2, __put_user_common() sends any size other than 1, 2 or 4 to
__copy_to_user().

Neither architecture selects HAVE_EFFICIENT_UNALIGNED_ACCESS, so
copy_from_kernel_nofault() and copy_to_kernel_nofault() (mm/maccess.c) use
the u64 loop whenever dst and src are both 8-byte aligned and at least 8
bytes remain:

    if (!(align & 7))
            copy_from_kernel_nofault_loop(dst, src, size, u64, Efault);

With this patch, every such access from a task with current->mm == NULL
(kworkers, khungtaskd, kernel_init before exec, the idle task in IRQ, kgdb
entered from idle) hits WARN_ON_ONCE(!current->mm), and __copy_from_user()
returns n.  copy_from_kernel_nofault() then returns -EFAULT on valid kernel
memory.

Concrete generic callers include:

  d_path() -> prepend() -> prepend_copy() (fs/d_path.c) fills the component
  with 'x' and stops the walk.  This is reachable from LSM/IMA path hooks
  run by kworkers, for example async firmware loading.

  print_worker_info() (kernel/workqueue.c), called from
  dump_stack_print_info() and sched_show_task() for kworkers, which copies
  wq->name and worker->desc.

  copy_to_kernel_nofault() from kgdb_hex2mem()/kgdb_ebin2mem() on nios2,
  which selects HAVE_ARCH_KGDB.

Before this patch these accesses worked on kernel addresses, since
raw_copy_{from,to}_user() on these architectures is a plain fixup-protected
copy and access_ok() is not involved.  Only opt-in fault injection could
make them fail.

The commit message rationale ("Accessing user memory from a task that has no
mm is always a bug") does not apply to kernel addresses.  The commit message
also says get_user()/put_user() are not covered, which does not hold for
8-byte accesses on these two architectures.

No later commit in this series appears to touch these paths.

[ ... ]

> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 5bb752ff7c61..565b3125e752 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -115,7 +115,7 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
>  	unsigned long max_addr, src_addr;
>  
>  	might_fault();
> -	if (should_fail_usercopy())
> +	if (uaccess_should_fail())
>  		return -EFAULT;
>  	if (unlikely(count <= 0))
>  		return 0;

Can strncpy_from_user_nofault() now trip this WARN_ON_ONCE() from contexts
where current->mm is NULL by design?

In this tree strncpy_from_user_nofault() (mm/maccess.c) has no current->mm
check and no nmi_uaccess_okay() check.  It goes straight to
strncpy_from_user():

    pagefault_disable();
    ret = strncpy_from_user(dst, unsafe_addr, count);
    pagefault_enable();

and strncpy_from_user() now calls uaccess_should_fail(), which does:

    if (WARN_ON_ONCE(!current->mm))
            return true;

The callers of strncpy_from_user_nofault() are tracing helpers that run in
whatever context the probe fires in, including kernel threads and IRQs taken
while the idle task runs:

  bpf_probe_read_user_str() via bpf_probe_read_user_str_common()
  (kernel/trace/bpf_trace.c), and bpf_probe_read_str() for addresses below
  TASK_SIZE.

  bpf_trace_printk()/bpf_snprintf() through bpf_trace_copy_string()
  (kernel/bpf/helpers.c).  With CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE
  (x86, arm64, ...), any '%s' argument below TASK_SIZE takes this path,
  including a NULL pointer.  So bpf_printk("%s", ptr) with ptr == NULL in a
  kprobe that fires in a kworker now triggers the warning.

  kprobe/fprobe/eprobe ustring fetch args (fetch_store_string_user() in
  kernel/trace/trace_probe_kernel.h).

  The .ustring event filters (test_ustring() in
  kernel/trace/trace_events_filter.c).

  Synthetic event string fields (trace_string() in
  kernel/trace/trace_events_synth.c).

  The bpf_copy_from_user_str_dynptr family (copy_user_str_nofault()).

Before this patch these calls either failed quietly with -EFAULT or read the
lazily loaded mm.  Now a tracing user (CAP_BPF plus CAP_PERFMON, or tracefs
access) can trigger a warning splat, and a panic with panic_on_warn, just by
tracing a function that kernel threads run.

The same applies to copy_to_user_iter_nofault() in lib/iov_iter.c, which this
patch also switches to uaccess_should_fail().  The commit message lists
copy_to_user_nofault() as intentionally covered, but it does not account for
the nofault helpers being used from mm-less contexts by design.

A fix appears later in the series, e8bf23b5b30d ("uaccess: Fail user nofault
helpers quietly when current->mm is NULL").  It adds an early !current->mm
return to copy_{from,to}_user_nofault(), strncpy_from_user_nofault() and
strnlen_user_nofault(), and switches copy_to_user_iter_nofault() back to
should_fail_usercopy().  Its message describes this exact problem
(bpf_probe_read_user_str() from a BPF program attached to a function called
by a kernel thread would now trip the WARN_ON_ONCE() in
strncpy_from_user()).

This commit on its own still introduces the regression, which breaks
bisection.  Could that fix be folded into this patch, or ordered before it?


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