Messages in this thread | | | Subject | Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls | From | Bartlomiej Zolnierkiewicz <> | Date | Thu, 14 May 2020 16:25:35 +0200 |
| |
On 5/14/20 4:07 PM, Al Viro wrote: > On Thu, May 14, 2020 at 03:45:09PM +0200, Bartlomiej Zolnierkiewicz wrote: >> >> Hi Al, >> >> On 5/10/20 1:45 AM, Al Viro wrote: >>> From: Al Viro <viro@zeniv.linux.org.uk> >>> >>> addresses passed only to get_user() and put_user() >> >> This driver lacks checks for {get,put}_user() return values so it will >> now return 0 ("success") even if {get,put}_user() fails. >> >> Am I missing something? > > "now" is interesting, considering > /* We let the MMU do all checking */ > static inline int access_ok(const void __user *addr, > unsigned long size) > { > return 1; > } > in arch/m68k/include/asm/uaccess_mm.h > > Again, access_ok() is *NOT* about checking if memory is readable/writable/there > in the first place. All it does is a static check that address is in > "userland" range - on architectures that have kernel and userland sharing the > address space. On architectures where we have separate ASI or equivalents > thereof for kernel and for userland the fscker is always true. > > If MMU will prevent access to kernel memory by uaccess insns for given address > range, access_ok() is fine with it. It does not do anything else. > > And yes, get_user()/put_user() callers should handle the fact that those can > fail. Which they bloody well can _after_ _success_ of access_ok(). And > without any races whatsoever. > > IOW, the lack of such checks is a bug, but it's quite independent from the > bogus access_ok() call. On any architecture. mmap() something, munmap() > it and pass the address where it used to be to that ioctl(). Failing > get_user()/put_user() is guaranteed, so's succeeding access_ok(). > > And that code is built only on amiga, so access_ok() always succeeds, anyway.
Thank you for in-detail explanations, for this patch:
Acked-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Could you also please take care of adding missing checks for {get,put}_user() failures later?
Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
| |