linux-kernel - Re: [PATCH 11/20] amifb: get rid of pointless access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200514140720.GB23230@ZenIV.linux.org.uk>
Date:   Thu, 14 May 2020 15:07:20 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 11/20] amifb: get rid of pointless access_ok() calls

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