Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

[Al Viro]

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