On Tue, Oct 27, 2026, at 18:29, James Houghton wrote: > On Mon, Oct 26, 2026 at 5:13 PM Arnd Bergmann <[email protected]> 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? - 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. >> 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 > 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. Arnd
