linux-kernel - Re: [PATCH] Convert filldir[64]() from __put_user() to unsafe_put

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]

Date:   Sun, 13 Oct 2019 20:59:49 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] Convert filldir[64]() from __put_user() to
 unsafe_put_user()

On Sun, Oct 13, 2019 at 12:22:38PM -0700, Linus Torvalds wrote:
> On Sun, Oct 13, 2019 at 12:10 PM Al Viro <viro@...iv.linux.org.uk> wrote:
> >
> > No arguments re put_user_ex side of things...  Below is a completely
> > untested patch for get_user_ex elimination (it seems to build, but that's
> > it); in any case, I would really like to see comments from x86 folks
> > before it goes anywhere.
> 
> Please don't do this:
> 
> > +       if (unlikely(__copy_from_user(&sc, usc, sizeof(sc))))
> > +               goto Efault;
> 
> Why would you use __copy_from_user()? Just don't.
> 
> > +       if (unlikely(__copy_from_user(&v, user_vm86,
> > +                       offsetof(struct vm86_struct, int_revectored))))
> 
> Same here.
> 
> There's no excuse for __copy_from_user().

Probably...  Said that, vm86 one is preceded by
        if (!access_ok(user_vm86, plus ?
                       sizeof(struct vm86_struct) :
                       sizeof(struct vm86plus_struct)))
                return -EFAULT;
so I didn't want to bother.  We'll need to eliminate most of
access_ok() anyway, and I figured that conversion to plain copy_from_user()
would go there as well.

Again, this is not a patch submission - just an illustration of what I meant
re getting rid of get_user_ex().  IOW, the whole thing is still in the
plotting stage.

Re plotting: how strongly would you object against passing the range to
user_access_end()?  Powerpc folks have a very close analogue of stac/clac,
currently buried inside their __get_user()/__put_user()/etc. - the same
places where x86 does, including futex.h and friends.

And there it's even costlier than on x86.  It would obviously be nice
to lift it at least out of unsafe_get_user()/unsafe_put_user() and
move into user_access_begin()/user_access_end(); unfortunately, in
one subarchitecture they really want it the range on the user_access_end()
side as well.  That's obviously not fatal (they can bloody well save those
into thread_info at user_access_begin()), but right now we have relatively
few user_access_end() callers, so the interface changes are still possible.

Other architectures with similar stuff are riscv (no arguments, same
as for stac/clac), arm (uaccess_save_and_enable() on the way in,
return value passed to uaccess_restore() on the way out) and s390
(similar to arm, but there it's needed only to deal with nesting,
and I'm not sure it actually can happen).

It would be nice to settle the API while there are not too many users
outside of arch/x86; changing it later will be a PITA and we definitely
have architectures that do potentially costly things around the userland
memory access; user_access_begin()/user_access_end() is in the right
place to try and see if they fit there...