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

[James Houghton <jthoughton@google.com>]

On Tue, Oct 27, 2026 at 11:32 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Oct 27, 2026, at 18:29, James Houghton wrote:
> > On Mon, Oct 26, 2026 at 5:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> 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.
>
> 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?

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.

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

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

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.

> > 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?:
>
> --- a/include/asm-generic/access_ok.h
> +++ b/include/asm-generic/access_ok.h
> @@ -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.

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.

Even the quiet 'if (!mm) return false' version would have missed the
KVM case, because of the first two points. I don't think there's really
a "small" fix here, unfortunately. :/

Thanks again for all the feedback, Arnd. I really hope we can get this
series into a mergeable state. :)

[1] https://lore.kernel.org/linux-mm/CADrL8HUZ3t8RdMEeBPfqsy1_1rN0Ty9B2c4iP6OzNS12te6vOg@mail.gmail.com/
[2] https://lore.kernel.org/kvm/auEVMrMDqVuKtDqh@google.com/