Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220209144910.1484686-1-arnd@kernel.org> <80c6df0717014472aa81093ae3894d39@AcuMS.aculab.com>
In-Reply-To: <80c6df0717014472aa81093ae3894d39@AcuMS.aculab.com>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Thu, 10 Feb 2022 14:29:59 +0100
Message-ID: <CAK8P3a3tZEc30AaiNENbHKf8+x5VOw7Q=4dVDMNwz0F6+v9YrQ@mail.gmail.com>
Subject: Re: [PATCH] microblaze: remove CONFIG_SET_FS
To:     David Laight <David.Laight@aculab.com>
Cc:     Michal Simek <monstr@monstr.eu>,
        Christoph Hellwig <hch@infradead.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 10, 2022 at 10:36 AM David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Sent: 09 February 2022 14:49
> >
> > Remove the address space override API set_fs().  The microblaze user
> > address space is now limited to TASK_SIZE.
> >
> > To support this we implement and wire in __get_kernel_nofault and
> > __set_kernel_nofault.
> >
> > The function user_addr_max is removed as there is a default definition
> > provided when CONFIG_SET_FS is not used.
> ...
> >  static inline int access_ok(const void __user *addr, unsigned long size)
> >  {
> >       if (!size)
> >               goto ok;
> >
> > -     if ((get_fs().seg < ((unsigned long)addr)) ||
> > -                     (get_fs().seg < ((unsigned long)addr + size - 1))) {
> > -             pr_devel("ACCESS fail at 0x%08x (size 0x%x), seg 0x%08x\n",
> > -                     (__force u32)addr, (u32)size,
> > -                     (u32)get_fs().seg);
> > +     if ((((unsigned long)addr) > TASK_SIZE) ||
> > +         (((unsigned long)addr + size - 1) > TASK_SIZE)) {
> > +             pr_devel("ACCESS fail at 0x%08x (size 0x%x)",
> > +                     (__force u32)addr, (u32)size);
> >               return 0;
>
> Isn't that the wrong check?
> If 'size' is big 'addr + size' can wrap.

Ah right, that seems dangerous, and we should probably fix that first, with
a backport to stable kernels before my patch. I see the same bug on csky
and hexagon:

static inline int __access_ok(unsigned long addr, unsigned long size)
{
        unsigned long limit = current_thread_info()->addr_limit.seg;
        return ((addr < limit) && ((addr + size) < limit));
}

#define __access_ok(addr, size) \
        ((get_fs().seg == KERNEL_DS.seg) || \
        (((unsigned long)addr < get_fs().seg) && \
          (unsigned long)size < (get_fs().seg - (unsigned long)addr)))

ia64 and sparc skip the size check entirely but rely on an unmapped page
at the beginning of the kernel address range, which avoids this problem
but may also be dangerous.

m68k-coldfire-mmu has a comment about needing a check, but tests
for neither address nor size.

> It needs to be (addr >= TASK_SIZE || size > TASK_SIZE - addr)
>
> Which is pretty much a generic version.
> Although typical 64bit architectures can use the faster:
>         ((addr | size) >> 62)

I think this is the best version, and it's already widely used:

static inline int __range_ok(unsigned long addr, unsigned long size)
{
        return size <= TASK_SIZE && addr <= (TASK_SIZE - size);
}

since 'size' is usually constant, so this turns into a single comparison
against a compile-time constant.

         Arnd