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

Reviewed-by Jens Axboe <[email protected]>

scsi_bsg_uring_task_cb() copies the sense data to the submitter's response_addr with copy_to_user(). It runs as io_uring task work, but io_uring may also run it with tw.cancel set from other contexts when the submitting task can't run it: from a kworker when the submitting task has exited (io_tctx_fallback_work()), or from io_cancel_local_task_work() when a DEFER_TASKRUN ring is torn down.

Quoted 2 lines

bpf: Fail sleepable user copy helpers when current->mm is NULL

TL;DR: Commit message only: the security_file_release() example of a sleepable program running from a kworker looks inaccurate, since such a program appears not to be attachable there. The fix itself looks justified.

Quoted 17 lines

bpf: Fail sleepable user copy helpers when current->mm is NULL

Sleepable BPF programs can run on behalf of a kernel thread, or of a task that has already gone through exit_mm(). For example, bpf_wq callbacks always run from a kworker, bpf_task_work callbacks queued late enough run from exit_task_work(), and lsm.s or fentry.s programs may be attached to functions called during exit_files(), exit_task_work() or from kernel threads (e.g. security_file_release() from a kworker running delayed_fput()).
In such contexts, bpf_copy_from_user(), bpf_copy_from_user_str() and bpf_copy_from_user{,_str}_dynptr() (when reading from the current task) call copy_from_user() or strncpy_from_user() without an mm. That access does not reliably fail: the user half of the address space belongs to whatever mm happens to be lazily loaded, if any, so the program may read another task's memory.
As there is no user memory to read from in this case, fail with -EFAULT without attempting the access.

Quoted 4 lines

Fixes 07be4c4a3e7a ("bpf: Add bpf_copy_from_user() helper.")

Fixes 65ab5ac4df01 ("bpf: Add bpf_copy_from_user_str kfunc")

Fixes a498ee7576de ("bpf: Implement dynptr copy kfuncs")

Signed-off-by James Houghton <[email protected]>

The commit message gives security_file_release() as an example of a place where an lsm.s or fentry.s program can run from a kworker:

Quoted 1 line

(e.g. security_file_release() from a kworker running delayed_fput())

Is this example accurate for the tree being patched?

bpf_lsm_file_release is not in the sleepable_lsm_hooks set in kernel/bpf/bpf_lsm.c, so btf_id_allow_sleepable() rejects a sleepable BPF_PROG_TYPE_LSM program there. bpf_lsm_file_free_security, the other hook reached from __fput() through file_free(), is in untrusted_lsm_hooks rather than the sleepable set.

For a BPF_PROG_TYPE_TRACING program, check_attach_sleepable() requires within_error_injection_list(addr) (or a syscall prefix when CONFIG_FUNCTION_ERROR_INJECTION is off), or an fmodret set with KF_SLEEPABLE. Neither security_file_release() nor bpf_lsm_file_release() has ALLOW_ERROR_INJECTION, and the only fmodret sets are in net/bpf/test_run.c, net/mptcp/bpf.c and bpf_testmod.

That suggests a program attached to security_file_release() is never sleepable and cannot call bpf_copy_from_user() or the KF_SLEEPABLE copy kfuncs.

The fix itself still looks justified, since bpf_wq callbacks run from a kworker and is_async_cb_sleepable() is true for them. Only the example appears to be off.

Would an example that is actually reachable keep the changelog accurate? For instance, bpf_lsm_task_alloc() when kthreadd forks a kernel thread (current is kthreadd), or bpf_lsm_kernel_read_file() and bpf_lsm_file_open() when a kworker loads firmware with kernel_read_file(). All three hooks are in sleepable_lsm_hooks.

Quoted 2 lines

stacktrace: Don't unwind the user stack without an mm

TL;DR: Commit message only: the Fixes: tag may point at the wrong commit; 7e8e6816c649, which dropped the !current->mm check, looks like the real culprit rather than 214d8ca6ee85.

Quoted 22 lines

stacktrace: Don't unwind the user stack without an mm

stack_trace_save_user() only refuses to unwind the user stack of tasks with PF_KTHREAD set. That doesn't cover all tasks without an mm:
- A user task that has already gone through exit_mm(), e.g. when an event fires from exit_files(), exit_task_work() or the final context switch in do_exit().
- PID 1 before it execs init, and usermode helper children before they exec, which are created with user_mode_thread() and don't have PF_KTHREAD set. E.g. booting with trace_options=userstacktrace and an event enabled with trace_event= unwinds PID 1's "user stack" during initcalls.
With the ftrace userstacktrace option enabled, the architecture unwinders then read the user stack without an mm. That access does not reliably fail: the user half of the address space belongs to whatever mm happens to be lazily loaded, if any, so the unwinder may read another task's memory.
Use is_user_task() instead, which also checks current->mm, as perf already does for the same reason since commit 76ed27608f7d ("perf:
sched: Fix perf crash with new is_user_task() helper"). This also stops unwinding the user stack of PF_USER_WORKER threads, which never run in user space, matching perf.

Quoted 2 lines

Fixes 214d8ca6ee85 ("stacktrace: Provide common infrastructure")

Signed-off-by James Houghton <[email protected]>

Is the Fixes: tag pointing at the right commit?

When 214d8ca6ee85 added the ARCH_STACKWALK version of stack_trace_save_user(), it guarded the unwind with:

        /* Trace user stack if not a kernel thread */
        if (!current->mm)
                return 0;

That check already covers both cases described above (a task past exit_mm(), and PID 1 or usermode helpers before exec, which have mm == NULL).

The check was replaced by commit 7e8e6816c649 ("stacktrace: Use PF_KTHREAD to check for kernel threads"), which switched it to:

        if (current->flags & PF_KTHREAD)
                return 0;

That dropped the no-mm protection, which looks like what this patch restores via is_user_task().

Should this be:

Fixes 7e8e6816c649 ("stacktrace: Use PF_KTHREAD to check for kernel threads")

get_perf_callchain() only checks is_user_task(current) when the regs it is given aren't user mode regs. However, BPF's bpf_get_task_stack() (and bpf_get_task_stack_sleepable()) pass task_pt_regs(current) when called on the current task, which are user mode regs for a user task, even if that task has already gone through exit_mm(), e.g. when called from a program attached to exit_files() or to the dying task's final sched_switch.

perf_callchain_user() then unwinds the user stack without an mm. That access does not reliably fail: the user half of the address space belongs to whatever mm happens to be lazily loaded, if any, so the unwinder may read another task's memory. This affects e.g. arm64, riscv, loongarch and csky, whose perf_callchain_user() doesn't check current->mm itself. perf events themselves are not affected, as perf_callchain() already clears the user callchain request when current is not a user task with an mm.

Check is_user_task(current) regardless of the regs' mode.

Fixes fa28dcb82a38 ("bpf: Introduce helper bpf_get_task_stack()")

Signed-off-by James Houghton <[email protected]>

---
 kernel/events/callchain.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

kernel/events/callchain.c +3 -3

index 9d24b6e0c91f..a451ca2a5573 100644
@@ -245,11 +245,15 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
 	}
 
 	if (user && !crosstask) {
-		if (!user_mode(regs)) {
-			if (!is_user_task(current))
-				goto exit_put;
+		/*
+		 * Even if @regs are user mode regs, e.g. task_pt_regs(current)
+		 * passed in by bpf_get_task_stack(), current may already have

Quoted 46 lines · James Houghton

get_perf_callchain() only checks is_user_task(current) when the regs it is given aren't user mode regs. However, BPF's bpf_get_task_stack() (and bpf_get_task_stack_sleepable()) pass task_pt_regs(current) when called on the current task, which are user mode regs for a user task, even if that task has already gone through exit_mm(), e.g. when called from a program attached to exit_files() or to the dying task's final sched_switch.

perf_callchain_user() then unwinds the user stack without an mm. That access does not reliably fail: the user half of the address space belongs to whatever mm happens to be lazily loaded, if any, so the unwinder may read another task's memory. This affects e.g. arm64, riscv, loongarch and csky, whose perf_callchain_user() doesn't check current->mm itself. perf events themselves are not affected, as perf_callchain() already clears the user callchain request when current is not a user task with an mm.

Check is_user_task(current) regardless of the regs' mode.

Fixes fa28dcb82a38 ("bpf: Introduce helper bpf_get_task_stack()")

Signed-off-by James Houghton <[email protected]>

---
 kernel/events/callchain.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

kernel/events/callchain.c +8 -4

index 9d24b6e0c91f..a451ca2a5573 100644
@@ -245,11 +245,15 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
      }
 
      if (user && !crosstask) {
-             if (!user_mode(regs)) {
-                     if (!is_user_task(current))
-                             goto exit_put;
+             /*
+              * Even if @regs are user mode regs, e.g. task_pt_regs(current)
+              * passed in by bpf_get_task_stack(), current may already have
+              * gone through exit_mm().
+              */
+             if (!is_user_task(current))
+                     goto exit_put;
+             if (!user_mode(regs))
                      regs = task_pt_regs(current);
-             }
 

kernel/events/callchain.c get_perf_callchain(struct pt_regs *regs, bool kernel, bool user, · line 259

Quoted 2 lines

kernel/events/callchain.c +0 -0

              if (defer_cookie) {
                      /*

We have perf_event_exit_task() before exit_mm().

Still, it doesn't hurt to be cautious. So no real objection, but perhaps adjust the Changelog?

Quoted 2 lines

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

Quoted 5 lines

include/linux/uaccess.h +0 -0

index eddbbb65ccc4..e54a6d804bf9 100644
@@ -50,6 +50,23 @@

[ ... ]

Quoted 6 lines

include/linux/uaccess.h +6 -0

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

[ ... ]

include/linux/uaccess.h __copy_from_user(void *to, const void __user *from, unsigned long n) · line 137

Quoted 9 lines

include/linux/uaccess.h +1 -1

@@ -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.

[ ... ]

lib/strncpy_from_user.c long strncpy_from_user(char *dst, const char __user *src, long count) · line 121

Quoted 13 lines

lib/strncpy_from_user.c +1 -1

index 5bb752ff7c61..565b3125e752 100644
@@ -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?

Quoted 2 lines

uaccess: Refuse and warn on __copy_from_user_inatomic() without an mm

TL;DR: Commit message only: it names kvm_read_guest_atomic(), which no longer exists in this tree; the review suggests kvm_vcpu_read_guest_atomic() instead.

Quoted 17 lines

uaccess: Refuse and warn on __copy_from_user_inatomic() without an mm

__copy_from_user_inatomic() never called should_fail_usercopy(), so the NULL current->mm check added to uaccess_should_fail() does not cover it. It is used e.g. by KVM's kvm_read_guest_atomic(), by several architectures' perf callchain and stack trace code, and by the generic copy_from_user_inatomic_nontemporal().

Split the NULL mm check out of uaccess_should_fail() into uaccess_without_mm(), and use it in __copy_from_user_inatomic() to warn and refuse the copy.

Don't use uaccess_should_fail() there, as that would also start injecting failures into __copy_from_user_inatomic(), which some callers don't expect to fail in practice, e.g. __wp_page_copy_user() warns if the copy fails twice.

Quoted 6 lines

copy_from_user_nofault() already fails quietly when current->mm is NULL, and the generic user stack unwinding entry points (perf_callchain(), get_perf_callchain() and stack_trace_save_user()) only unwind the user stack of a user task with an mm, so neither is affected.

Signed-off-by James Houghton <[email protected]>

The changelog names kvm_read_guest_atomic() as a user of __copy_from_user_inatomic(), but that function no longer exists in this tree. It was removed by commit ef82eddc0e31 ("KVM: Remove kvm_read_guest_atomic()").

The KVM user is now kvm_vcpu_read_guest_atomic() in virt/kvm/kvm_main.c, which calls the static __kvm_read_guest_atomic():

    static int __kvm_read_guest_atomic(struct kvm_memory_slot *slot, gfn_t gfn,
                                       void *data, int offset, unsigned long len)
    {
        ...
        pagefault_disable();
        r = __copy_from_user_inatomic(data, (void __user *)addr + offset, len);
        pagefault_enable();

A grep for kvm_read_guest_atomic only finds __kvm_read_guest_atomic() and kvm_vcpu_read_guest_atomic().

Could the changelog say kvm_vcpu_read_guest_atomic() instead?

uaccess_without_mm() is inlined into every user copy, including every copy_{to,from}_user() call site on architectures that define INLINE_COPY_USER, such as arm64. Each inlined WARN_ON_ONCE() adds its own __bug_table entry.

Keep the NULL current->mm check inline, but move the warning into a single out-of-line, __cold function, uaccess_warn_without_mm().

The trade-off is that only the first offending access in the system warns, rather than the first one from each call site. The stack trace still identifies the caller, and the access is refused either way.

Compared to the base of this series, with defconfig + KVM:

inline WARN_ON_ONCE() out-of-line warning
x86_64 total +5,884 B (+0.014%) +9,714 B (+0.023%)
arm64 total +40,948 B (+0.10%) +24,609 B (+0.06%)

On arm64, this removes the ~16 KB of __bug_table growth, while text size is unchanged, as a call is the same size as the inline brk. On x86_64, the call is larger than the inline ud2 and causes more .cold splitting, so this is a slight regression.

Signed-off-by James Houghton <[email protected]>

---
NOTE: This patch is included for reference only, to show the cost of the
inline warning.

Quoted 23 lines · James Houghton

uaccess_without_mm() is inlined into every user copy, including every copy_{to,from}_user() call site on architectures that define INLINE_COPY_USER, such as arm64. Each inlined WARN_ON_ONCE() adds its own __bug_table entry.

Keep the NULL current->mm check inline, but move the warning into a single out-of-line, __cold function, uaccess_warn_without_mm().

The trade-off is that only the first offending access in the system warns, rather than the first one from each call site. The stack trace still identifies the caller, and the access is refused either way.

Compared to the base of this series, with defconfig + KVM:

inline WARN_ON_ONCE() out-of-line warning
x86_64 total +5,884 B (+0.014%) +9,714 B (+0.023%)
arm64 total +40,948 B (+0.10%) +24,609 B (+0.06%)

On arm64, this removes the ~16 KB of __bug_table growth, while text size is unchanged, as a call is the same size as the inline brk. On x86_64, the call is larger than the inline ud2 and causes more .cold splitting, so this is a slight regression.

Quoted 4 lines

Signed-off-by James Houghton <[email protected]>

---
NOTE: This patch is included for reference only, to show the cost of the
inline warning.

I would suspect that the growth is larger on architectures that don't implement __WARN_FLAGS() but rely on the generic WARN_ON().

This would mainly affect 32-bit arm and mips machines, some of which are very memory limited, most of the 64-bit architectures have it.

50% of the size increase for arm and mips seems worth the trade-off

This series comes out of a set of bugs that were found in KVM where the kernel can sometimes write to a random process's user memory[1]. The KVM fixes at [1] are still required, even with this uaccess series, as noted by Sean here[2] and below in this cover letter. The beginning of this series includes fixes for similar bugs found through static analysis with Claude Opus 5.5. I don't think any of these bugs are more severe than the KVM bugs[1].

We hit one of the KVM bugs in production, and it was challenging to debug. This series makes such bugs, where the kernel is doing uaccess without an mm, much more obvious.

Such an access does not reliably fail. The user portion of the address space belongs to whatever mm happens to be lazily loaded, if any. If the access faults, the fault handler refuses to handle it and the copy fails, but if the page is already mapped, the access silently reads or writes another task's memory.

== The New WARN ==

The rest of the series makes the generic uaccess helpers WARN_ON_ONCE() and refuse the access when current->mm is NULL:

- Patch 6 adds uaccess_should_fail(), which combines the existing should_fail_usercopy() fault injection check with the NULL mm check, and uses it in place of should_fail_usercopy(). This covers copy_{to,from}_user() on all architectures (including the masked user access paths, which never call access_ok()), the __copy_{to,from}_user() variants used by KVM's guest memory accessors, the user-backed iov_iter copy routines and strncpy_from_user().

The checks are deliberately limited to generic code. Accesses implemented entirely by architectures (get_user(), put_user(), clear_user(), and direct users of user_access_begin() or masked_user_access_begin()) are not covered. Please let me know if the new check should be placed elsewhere.

E.g. on arm64 with CONFIG_ARM64_SW_TTBR0_PAN, the page table base for the current process is only set in user_access_begin() and would not point to any task here. Without ttbr0_pan, it should be covered by cpu_set_reserved_ttbr0() turning off all user space access for kernel threads.

Quoted 9 lines · James Houghton

This series comes out of a set of bugs that were found in KVM where the kernel can sometimes write to a random process's user memory[1]. The KVM fixes at [1] are still required, even with this uaccess series, as noted by Sean here[2] and below in this cover letter. The beginning of this series includes fixes for similar bugs found through static analysis with Claude Opus 5.5. I don't think any of these bugs are more severe than the KVM bugs[1].

Quoted 3 lines

We hit one of the KVM bugs in production, and it was challenging to debug. This series makes such bugs, where the kernel is doing uaccess without an mm, much more obvious.

Nice work!

Quoted 5 lines

Such an access does not reliably fail. The user portion of the address space belongs to whatever mm happens to be lazily loaded, if any. If the access faults, the fault handler refuses to handle it and the copy fails, but if the page is already mapped, the access silently reads or writes another task's memory.

This sounds like the problem depends a lot on the architecture, and to what degree they implement lazy mm switching. If an architecture makes all user pointers invalid for a kernel thread, this does not actually happen, right?

E.g. on arm64 with CONFIG_ARM64_SW_TTBR0_PAN, the page table base for the current process is only set in user_access_begin() and would not point to any task here. Without ttbr0_pan, it should be covered by cpu_set_reserved_ttbr0() turning off all user space access for kernel threads.

Should this perhaps be addressed in architecture specific code instead of in a common place?

Quoted 13 lines

== The New WARN ==

The rest of the series makes the generic uaccess helpers WARN_ON_ONCE() and refuse the access when current->mm is NULL:

- Patch 6 adds uaccess_should_fail(), which combines the existing should_fail_usercopy() fault injection check with the NULL mm check, and uses it in place of should_fail_usercopy(). This covers copy_{to,from}_user() on all architectures (including the masked user access paths, which never call access_ok()), the __copy_{to,from}_user() variants used by KVM's guest memory accessors, the user-backed iov_iter copy routines and strncpy_from_user().

...

Quoted 5 lines

The checks are deliberately limited to generic code. Accesses implemented entirely by architectures (get_user(), put_user(), clear_user(), and direct users of user_access_begin() or masked_user_access_begin()) are not covered. Please let me know if the new check should be placed elsewhere.

I would think that user_access_begin() is a more natural place for the added checks, as this should be called exactly once for any function that accesses user space and also stays out of the way of the copy_from_kernel_nofault() etc.

Quoted 9 lines · Arnd Bergmann

E.g. on arm64 with CONFIG_ARM64_SW_TTBR0_PAN, the page table base for the current process is only set in user_access_begin() and would not point to any task here. Without ttbr0_pan, it should be covered by cpu_set_reserved_ttbr0() turning off all user space access for kernel threads.

Should this perhaps be addressed in architecture specific code instead of in a common place?

You're right that whether corruption can actually occur depends on the architecture and config. I'll write a note about this in the v2 cover letter.

That being said, I still think the right thing to do is to have the new check(s) in common code. Instances of improper uaccesses are still bugs, even if no corruption occurs.

Quoted 4 lines

I would think that user_access_begin() is a more natural place for the added checks, as this should be called exactly once for any function that accesses user space and also stays out of the way of the copy_from_kernel_nofault() etc.

user_access_begin() isn't used in a lot of places that I want to catch, including the KVM case. Roughly:

kvm_write_guest_cached() --> __copy_to_user() --> raw_copy_to_user()

There's no user_access_begin() in there. :/

I do think user_access_begin() should have the new check though, so I've added it in the v2.

Quoted 11 lines · James Houghton

Quoted 4 lines

I would think that user_access_begin() is a more natural place for the added checks, as this should be called exactly once for any function that accesses user space and also stays out of the way of the copy_from_kernel_nofault() etc.

user_access_begin() isn't used in a lot of places that I want to catch, including the KVM case. Roughly:

kvm_write_guest_cached() --> __copy_to_user() --> raw_copy_to_user()

There's no user_access_begin() in there. :/

Good point, yes. After the changes described in
https://lwn.net/Articles/1102061/, I had assumed that all __copy_{from,to}_user() are going from common code, but it looks like the series so far only touches __{get,put}_user().

There are of course few enough of these left that I think we can consider doing the same thing here and turn them all into the checking version:

$ git grep -wl __copy_'\(from\|to\)'_user  drivers mm virt
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
drivers/gpu/drm/i915/i915_gem.c
drivers/gpu/drm/i915/i915_query.c
drivers/usb/gadget/udc/atmel_usba_udc.c
drivers/vhost/scsi.c
drivers/vhost/vhost.c
mm/kasan/kasan_test_c.c
virt/kvm/kvm_main.c

Quoted 2 lines

I do think user_access_begin() should have the new check though, so I've added it in the v2.

I had the details confused here. Seing that user_access_begin() and the other accessors always call access_ok() at some point, maybe that would be enough?:

include/asm-generic/access_ok.h +3 -0

@@ -33,6 +33,9 @@ static inline int __access_ok(const void __user *ptr, unsigned long size)
        unsigned long limit = TASK_SIZE_MAX;
        unsigned long addr = (unsigned long)ptr;
 
+       if (WARN_ONCE(!current->mm))
+               return false;
+
        if (IS_ENABLED(CONFIG_ALTERNATE_USER_ADDRESS_SPACE) ||
            !IS_ENABLED(CONFIG_MMU))
                return true;

(plus the few extra copies of access_ok()/__access_ok())

As a quick fix I would probably start with just the 'if (!mm) return false' bit here to get something merged while we still discuss the other details.

Quoted 11 lines · James Houghton

Quoted 9 lines · Arnd Bergmann

E.g. on arm64 with CONFIG_ARM64_SW_TTBR0_PAN, the page table base for the current process is only set in user_access_begin() and would not point to any task here. Without ttbr0_pan, it should be covered by cpu_set_reserved_ttbr0() turning off all user space access for kernel threads.

Should this perhaps be addressed in architecture specific code instead of in a common place?

Quoted 7 lines

You're right that whether corruption can actually occur depends on the architecture and config. I'll write a note about this in the v2 cover letter.

That being said, I still think the right thing to do is to have the new check(s) in common code. Instances of improper uaccesses are still bugs, even if no corruption occurs.

I agree we have to do something to prevent the problem for most configurations and find any newly introduced bugs. The questions that need to be properly decided are

- does there need to be any compile-time configuration to skip the extra checks and/or the warnings?

- are there any cheaper ways to make it safe than to add a condition on each user access?
E.g. the original lazy mm switch was intended to avoid expensive TLB flushes when switching to a kernel thread and back to the original task on 32-bit x86. On modern CPUs, the MMU can deal with multiple ASIDs cheaply, so turning off user access in a kernel thread may be cheaper than adding code to uaccess depending on the architecture.

Quoted 11 lines

Quoted 4 lines

I would think that user_access_begin() is a more natural place for the added checks, as this should be called exactly once for any function that accesses user space and also stays out of the way of the copy_from_kernel_nofault() etc.

user_access_begin() isn't used in a lot of places that I want to catch, including the KVM case. Roughly:

kvm_write_guest_cached() --> __copy_to_user() --> raw_copy_to_user()

There's no user_access_begin() in there. :/

Good point, yes. After the changes described in
https://lwn.net/Articles/1102061/, I had assumed that all __copy_{from,to}_user() are going from common code, but it looks like the series so far only touches __{get,put}_user().

There are of course few enough of these left that I think we can consider doing the same thing here and turn them all into the checking version:

$ git grep -wl __copy_'\(from\|to\)'_user  drivers mm virt
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
drivers/gpu/drm/i915/i915_gem.c
drivers/gpu/drm/i915/i915_query.c
drivers/usb/gadget/udc/atmel_usba_udc.c
drivers/vhost/scsi.c
drivers/vhost/vhost.c
mm/kasan/kasan_test_c.c
virt/kvm/kvm_main.c

Quoted 2 lines

I do think user_access_begin() should have the new check though, so I've added it in the v2.

I had the details confused here. Seing that user_access_begin() and the other accessors always call access_ok() at some point, maybe that would be enough?:

include/asm-generic/access_ok.h +3 -0

@@ -33,6 +33,9 @@ static inline int __access_ok(const void __user *ptr, unsigned long size)
        unsigned long limit = TASK_SIZE_MAX;
        unsigned long addr = (unsigned long)ptr;
 
+       if (WARN_ONCE(!current->mm))
+               return false;
+
        if (IS_ENABLED(CONFIG_ALTERNATE_USER_ADDRESS_SPACE) ||
            !IS_ENABLED(CONFIG_MMU))
                return true;

(plus the few extra copies of access_ok()/__access_ok())

As a quick fix I would probably start with just the 'if (!mm) return false' bit here to get something merged while we still discuss the other details.

Quoted 3 lines

- does there need to be any compile-time configuration to skip the extra checks and/or the warnings?

Since patch 10 moves the warning out of line, it costs nothing on the fast path, so making the WARN configurable wouldn't save anything. The question is really whether the mm check itself should be configurable, and that check is also what makes the access safe.

The text size increase seems manageable[1], but it would be great to hear other people's thoughts. For the runtime cost, I'll do some performance testing. My guess is that a load of current->mm and a well-predicted branch per user access will be negligible, but I'll get numbers.

Quoted 8 lines

- are there any cheaper ways to make it safe than to add a condition on each user access?
E.g. the original lazy mm switch was intended to avoid expensive TLB flushes when switching to a kernel thread and back to the original task on 32-bit x86. On modern CPUs, the MMU can deal with multiple ASIDs cheaply, so turning off user access in a kernel thread may be cheaper than adding code to uaccess depending on the architecture.

That's a fair point. If user memory were inaccessible in kernel threads and after exit_mm(), every such access would fault, and the fault handler could WARN about it (x86's do_user_addr_fault() already has a !mm branch), so it could still be loud without a check on every access.

I'll look into disabling lazy mm switching on x86 and doing some performance testing with that (like comparing the per-uaccess check with the added cost of non-lazy mm switching).

Other than the performance difference, I see these issues with making uaccess safe entirely with the mm switch mechanics:

- It only helps on architectures (and configs) that implement it, while the common check works everywhere today.

- The fault handler would need to tell legitimate faults apart from bugs, e.g. copy_from_user_nofault() from BPF or perf in a kernel thread is fine and must stay quiet.

The series handles that today by
checking current->mm in the nofault helpers (patch 6).

The two approaches are mostly complementary: if non-lazy switching turns out to be cheap enough, an architecture that does this will have better defense against uaccess bugs. But I'd still like the common check so that these bugs are reported on every architecture.

Quoted 4 lines

There are of course few enough of these left that I think we can consider doing the same thing here and turn them all into the checking version:

Quoted 9 lines

$ git grep -wl __copy_'\(from\|to\)'_user  drivers mm virt
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
drivers/gpu/drm/i915/i915_gem.c
drivers/gpu/drm/i915/i915_query.c
drivers/usb/gadget/udc/atmel_usba_udc.c
drivers/vhost/scsi.c
drivers/vhost/vhost.c
mm/kasan/kasan_test_c.c
virt/kvm/kvm_main.c

Agreed, I think we should clean those up, but that can be done separately. This series already adds the check to the common __copy_{from,to}_user() and __copy_{from,to}_user_inatomic() wrappers in include/linux/uaccess.h (patches 7 and 8), so the KVM path is covered without converting the callers first.

Quoted 3 lines

As a quick fix I would probably start with just the 'if (!mm) return false' bit here to get something merged while we still discuss the other details.

Making the same point as Sean[2] but with a little more detail, I had thought about making the change in access_ok(), but:

- On x86-64 and powerpc, the masked user access paths don't call access_ok() at all. That includes copy_from_user(), strncpy_from_user(), strnlen_user() and every scoped_user_*_access() block, which is where many of the converted __get_user() users ended up.

- __copy_{from,to}_user() and __copy_from_user_inatomic() skip it too, and the KVM bug followed exactly that pattern: the range was checked once up front, and the access came much later from a different context.

- access_ok() is also used as a plain range check where having no mm is fine. For example, copy_{from,to}_user_nofault() call __access_ok()/access_ok(), and BPF and perf legitimately call those from kernel threads, so a WARN there would fire on valid tracing.