linux-kernel - Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

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: <20200831173112.fjfnnup5cfc5t226@treble>
Date:   Mon, 31 Aug 2020 12:31:12 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "'x86@...nel.org'" <x86@...nel.org>,
        "'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>,
        'Linus Torvalds' <torvalds@...ux-foundation.org>,
        'Al Viro' <viro@...iv.linux.org.uk>,
        'Will Deacon' <will@...nel.org>,
        'Dan Williams' <dan.j.williams@...el.com>,
        'Andrea Arcangeli' <aarcange@...hat.com>,
        'Waiman Long' <longman@...hat.com>,
        'Peter Zijlstra' <peterz@...radead.org>,
        'Thomas Gleixner' <tglx@...utronix.de>,
        'Andrew Cooper' <andrew.cooper3@...rix.com>,
        'Andy Lutomirski' <luto@...nel.org>,
        'Christoph Hellwig' <hch@....de>
Subject: Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess
 speculation

On Sat, Aug 29, 2020 at 07:31:20PM +0000, David Laight wrote:
> > Rereading the patch it looks like a lot of bloat (as well as a
> > lot of changes).
> > Does the array_mask even work on 32bit archs where the kernel
> > base address is 0xc0000000?

Why wouldn't it on work on 32-bit?  My patch does have a minor compile
bug on 32-bit, but otherwise it seems to work (i.e., the asm looks ok,
and it boots).

> > I'm sure there is something much simpler.
> > 
> > If access_ok() generates ~0u or 0 without a conditional then
> > the address can be masked with the result.
> > So you probably need to change access_ok() to take the address
> > of the user pointer - so the callers become like:
> > 	if (access_ok(&user_buffer, len))
> > 		return -EFAULT
> > 	__put_user(user_buffer, value);
> > 
> > It would be easier if NULL were guaranteed to be an invalid
> > user address (is it?).
> > Then access_ok() could return the modified pointer.
> > So you get something like:
> > 	user_buffer = access_ok(user_buffer, len);
> > 	if (!user_buffer)
> > 		return -EFAULT.
> > 
> > Provided the 'last' user page is never allocated (it can't
> > be on i386 due to cpu prefetch issues) something like:
> > (and with the asm probably all broken)
> > 
> > static inline void __user * access_ok(void __user *b, size_t len)
> > {
> > 	unsigned long x = (long)b | (long)(b + len);
> > 	unsigned long lim = 64_bit ? 1u << 63 : 0x40000000;
> > 	asm volatile (" add %1, %0\n"
> > 			" sbb $0, %0", "=r" (x), "r" (lim));
> > 	return (void __user *)(long)b & ~x);
> > }
> 
> Actually, thinking further, if:
> 1) the access_ok() immediately precedes the user copy (as it should).
> 2) the user-copies use a sensible 'increasing address' copy.
> and
> 3) there is a 'guard page' between valid user and kernel addresses.
> Then access_ok() only need check the base address of the user buffer.

Yes, it would make sense to put the masking in access_ok() somehow.  But
to do it properly, I think we'd first need to make access_ok() generic.
Maybe that's do-able, but it would be a much bigger patch set.

First I'd prefer to just fix x86, like my patch does.  Then we could do
an access_ok() rework.

-- 
Josh