Re: [git pull] uaccess-related bits of vfs.git

Al Viro
2017-05-13T16:46:20Z

On Sat, May 13, 2017 at 02:05:27PM +0200, Adam Borowski wrote:

> As someone from the peanuts gallery, I took a look for __put_user() in my
> usual haunt, drivers/tty/vt/
>
> * use 1: con_[gs]et_trans_*():
> Copies a linear array of 256 bytes/shorts, one by one.
> The obvious patch has 9 insertions(+), 22 deletions(-).
>
> * use 2: con_[gs]et_unimap():
> Ditto, up to 65535*2 shorts, also in a nice linear array.
>
> * use 3: tioclinux():
> Does a __put into a place that was checked only for read. This got me
> frightened as it initially looked like something that can allow an user to
> write where they shouldn't. Fortunately, it turns out the first argument
> to access_ok() is ignored on every single architecture -- why does it even
> exist then? I imagined it's there for some odd arch that allows writes
> when in privileged mode, but unlike what the docs say, VERIFY_WRITE is
> exactly same as VERIFY_READ.

It's a remnant of old kludge that never properly worked in the first place.
access_ok() should have been called userland_range() - that's all it
checks and that's all it *can* check.

As it is, each of those __get_user() can bloody well yield -EFAULT. Despite
having "passed" access_ok(). Again, the only thing access_ok() checks is
that (on architecture with shared address space for userland and kernel)
the addresses given are on the userland side. That's _it_ - they can
be unmapped, mmapped to broken floppy, whatever; you'll find out when
you try to actually copy bytes from from it.

What the kludge used to attempt was "let's check that we are not trying
to copy into read-only mmapped area - 80386 MMU is fucked in head and won't
fault on such stores in ring 0". It had always been racy. Look:
thread A: going to copy something to user-supplied address. Do access_ok().
thread A: looks like it's mapped writable, let's go ahead and copy
thread A: do something blocking before actually doing __put_user() or
__copy_to_user() or whatever it's going to be.
thread B: munmap(), mmap() something read-only here.
thread A: get to actual __put_user()/__copy_to_user().
80386: hey, it's ring 0, no need to look at the write protect bit in page
tables. What can go wrong, anyway?

You can't move any non-static checks to access_ok(). On any architecture.
Anything that could change between access_ok() and actual copying can't
be checked in access_ok(). As it is, access_ok() has actively misleading
calling conventions:
* the name implies that having passed access_ok() you don't have
to worry about EFAULT
* 'direction' argument of that thing reinforces that impression
*and* has to be produced by the caller. Most simply pass a constant,
which immediately gets dropped (as an aside, take a look at 4b4554f6d - it's
amusing), but in some cases it's calculated elsewhere and carefully passed
through several levels of call chain. Only to be discarded by access_ok(),
of course...

> Ie, every use in this sample is wrong. I suspect the rest of the kernel
> should be similar.

Looking through vt...

* con_set_trans_old(): copy_from_user() + loop for doing or with
UNI_DIRECT_BASE. Almost certainly will be faster that way - on *any*
architecture.
* con_get_trans_old(): copy_to_user() would be an obvious optimization.
* con_set_trans_new(): copy_from_user().
* con_get_trans_new(): copy_to_user().
* con_set_unimap(): memdup_user() instead of the entire kmalloc_array +
loop copying the sucker member by member. With ushort ct you don't need
overflow-related logics of kmalloc_array() anyway...
* con_get_unimap(): copy_to_user() + put_user() (for uct)
* set_selection(): just copy_from_user() into local struct tiocl_selection.
* tioclinux(): use put_user(). Yes, you will repeat the same check twice.
Once per ioctl(), that involves enough work to make that "recalculate
access_ok() once for no reason" non-issue on any architecture.
* vt_ioctl(): turn that ushort ll,cc,vlin,clin,vcol,ccol; into
struct vt_consize size; or something like that and use copy_from_user()

And AFAICS you can lose each and every access_ok() call in there.