Date:   Tue, 15 Feb 2022 13:28:59 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Arnd Bergmann' <arnd@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>
CC:     Christoph Hellwig <hch@....de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Linux API <linux-api@...r.kernel.org>,
        "Arnd Bergmann" <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Will Deacon <will@...nel.org>, Guo Ren <guoren@...nel.org>,
        Brian Cain <bcain@...eaurora.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Michal Simek <monstr@...str.eu>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Nick Hu <nickhu@...estech.com>,
        Greentime Hu <green.hu@...il.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Stafford Horne <shorne@...il.com>,
        Helge Deller <deller@....de>,
        Michael Ellerman <mpe@...erman.id.au>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Rich Felker <dalias@...c.org>,
        David Miller <davem@...emloft.net>,
        Richard Weinberger <richard@....at>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Max Filippov <jcmvbkbc@...il.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        alpha <linux-alpha@...r.kernel.org>,
        "open list:SYNOPSYS ARC ARCHITECTURE" 
        <linux-snps-arc@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "linux-csky@...r.kernel.org" <linux-csky@...r.kernel.org>,
        "open list:QUALCOMM HEXAGON..." <linux-hexagon@...r.kernel.org>,
        "linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        "open list:BROADCOM NVRAM DRIVER" <linux-mips@...r.kernel.org>,
        Openrisc <openrisc@...ts.librecores.org>,
        Parisc List <linux-parisc@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        linux-s390 <linux-s390@...r.kernel.org>,
        Linux-sh list <linux-sh@...r.kernel.org>,
        sparclinux <sparclinux@...r.kernel.org>,
        linux-um <linux-um@...ts.infradead.org>,
        "open list:TENSILICA XTENSA PORT (xtensa)" 
        <linux-xtensa@...ux-xtensa.org>
Subject: RE: [PATCH 09/14] m68k: drop custom __access_ok()

From: Arnd Bergmann
> Sent: 15 February 2022 10:02
> 
> On Tue, Feb 15, 2022 at 8:13 AM Al Viro <viro@...iv.linux.org.uk> wrote:
> > On Tue, Feb 15, 2022 at 07:29:42AM +0100, Christoph Hellwig wrote:
> > > On Tue, Feb 15, 2022 at 12:37:41AM +0000, Al Viro wrote:
> > > > Perhaps simply wrap that sucker into #ifdef CONFIG_CPU_HAS_ADDRESS_SPACES
> > > > (and trim the comment down to "coldfire and 68000 will pick generic
> > > > variant")?
> > >
> > > I wonder if we should invert CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE,
> > > select the separate address space config for s390, sparc64, non-coldfire
> > > m68k and mips with EVA and then just have one single access_ok for
> > > overlapping address space (as added by Arnd) and non-overlapping ones
> > > (always return true).
> >
> > parisc is also such...  How about
> >
> >         select ALTERNATE_SPACE_USERLAND
> >
> > for that bunch?
> 
> Either of those works for me. My current version has this keyed off
> TASK_SIZE_MAX==ULONG_MAX, but a CONFIG_ symbol does
> look more descriptive.
> 
> >  While we are at it, how many unusual access_ok() instances are
> > left after this series?  arm64, itanic, um, anything else?
> 
> x86 adds a WARN_ON_IN_IRQ() check in there.

If is a noop unless CONFIG_DEBUG_ATOMIC_SLEEP is set.
I doubt that is often enabled.

> This could be
> made generic, but it's not obvious what exactly the exceptions are
> that other architectures need. The arm64 tagged pointers could
> probably also get integrated into the generic version.
> 
> > FWIW, sparc32 has a slightly unusual instance (see uaccess_32.h there); it's
> > obviously cheaper than generic and I wonder if the trick is legitimate (and
> > applicable elsewhere, perhaps)...
> 
> Right, a few others have the same, but I wasn't convinced that this
> is actually safe for call possible cases: it's trivial to construct a caller
> that works on other architectures but not this one, if you pass a large
> enough size value and don't access the contents in sequence.

You'd need code that did an access_ok() check and then read from
a large offset from the address - unlikely.
It's not like the access_ok() check for read/write is done on syscall
entry and then everything underneath assumes it is valid.

Hasn't (almost) everything been checked for function calls between
user_access_begin() and the actual accesses?
And access_ok() is done by/at the same time as user_access_begin()?

You do need an unmapped page above the address that is tested.

> Also, like the ((addr | (addr + size)) & MASK) check on some other
> architectures, it is less portable because it makes assumptions about
> the actual layout beyond a fixed address limit.

Isn't that test broken without a separate bound check on size?

I also seem to remember that access_ok(xxx, 0) is always 'ok'
and some of the 'fast' tests give a false negative if the user
buffer ends with the last byte of user address space.

So you may need:
	size < TASK_SIZE && (addr < (TASK_SIZE - size - 1) || !size)
(sprinkled with [un]likely())

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists