On Tue, Oct 27, 2026, Arnd Bergmann wrote: > On Tue, Oct 27, 2026, at 18:29, James Houghton wrote: > >> 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?:
It would help, but it wouldn't cover everything, e.g. KVM "caches" the access_ok() checks well in advance of actually performing a user access. I don't know that I would fixate on KVM too much though, as KVM's memslots infrastructure that begets that behavior is quite unique, i.e. it's unlikely other subsystems will have these types of bugs *and* elide access_ok() checks at the time of access. And KVM has natural chokepoints for these accesses, so it's easy enough to harden KVM itself. The counter-argument is that _if_ there are other subsystems that are caching access_ok() checks, then those are probably the subsystems that are most likely to mess this up and thus would benefit the most from common hardening. > --- 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.
