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]

Message-ID: <18992f7f25b44f2898812ffc203c4b35@AcuMS.aculab.com>
Date:   Tue, 8 Oct 2019 09:58:24 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Linus Torvalds' <torvalds@...ux-foundation.org>
CC:     Al Viro <viro@...iv.linux.org.uk>,
        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()

From: Linus Torvalds <torvalds@...ux-foundation.org>
> Sent: 07 October 2019 19:11
...
> I've been very close to just removing __get_user/__put_user several
> times, exactly because people do completely the wrong thing with them
> - not speeding code up, but making it unsafe and buggy.

They could do the very simple check that 'user_ptr+size < kernel_base'
rather than the full window check under the assumption that access_ok()
has been called and that the likely errors are just overruns.

> The new "user_access_begin/end()" model is much better, but it also
> has actual STATIC checking that there are no function calls etc inside
> the region, so it forces you to do the loop properly and tightly, and
> not the incorrect "I checked the range somewhere else, now I'm doing
> an unsafe copy".
> 
> And it actually speeds things up, unlike the access_ok() games.

I've code that does:
	if (!access_ok(...))
		return -EFAULT;
	...
	for (...) {
		if (__get_user(tmp_u64, user_ptr++))
			return -EFAULT;
		writeq(tmp_u64, io_ptr++);
	}
(Although the code is more complex because not all transfers are multiples of 8 bytes.)

With user_access_begin/end() I'd probably want to put the copy loop
inside a function (which will probably get inlined) to avoid convoluted
error processing.
So you end up with:
	if (!user_access_ok())
		return _EFAULT;
	user_access_begin();
	rval = do_copy_code(...);
	user_access_end();
	return rval;
Which, at the source level (at least) breaks your 'no function calls' rule.
The writeq() might also break it as well.

	David

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