From: Josh Poimboeuf <jpoimboe@redhat.com>
To: David Laight <David.Laight@aculab.com>
Cc: "'x86@kernel.org'" <x86@kernel.org>,
"'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>,
'Linus Torvalds' <torvalds@linux-foundation.org>,
'Al Viro' <viro@zeniv.linux.org.uk>,
'Will Deacon' <will@kernel.org>,
'Dan Williams' <dan.j.williams@intel.com>,
'Andrea Arcangeli' <aarcange@redhat.com>,
'Waiman Long' <longman@redhat.com>,
'Peter Zijlstra' <peterz@infradead.org>,
'Thomas Gleixner' <tglx@linutronix.de>,
'Andrew Cooper' <andrew.cooper3@citrix.com>,
'Andy Lutomirski' <luto@kernel.org>,
'Christoph Hellwig' <hch@lst.de>
Subject: Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation
Date: Mon, 31 Aug 2020 12:31:12 -0500 [thread overview]
Message-ID: <20200831173112.fjfnnup5cfc5t226@treble> (raw)
In-Reply-To: <56488b800d044a7e81efd8b40198a527@AcuMS.aculab.com>
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
next prev parent reply other threads:[~2020-08-31 17:31 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-19 14:50 [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation Josh Poimboeuf
2020-08-19 16:39 ` Andy Lutomirski
2020-08-19 17:02 ` Josh Poimboeuf
2020-08-19 21:30 ` David Laight
2020-08-20 0:18 ` Andy Lutomirski
2020-08-28 19:29 ` Josh Poimboeuf
2020-08-29 13:21 ` David Laight
2020-08-29 19:31 ` David Laight
2020-08-31 17:31 ` Josh Poimboeuf [this message]
2020-09-01 8:32 ` David Laight
2020-09-01 14:26 ` Josh Poimboeuf
2020-09-01 15:00 ` David Laight
2020-09-01 15:24 ` Josh Poimboeuf
2020-09-01 14:02 ` Mark Rutland
2020-09-01 14:21 ` Josh Poimboeuf
2020-09-01 14:52 ` Mark Rutland
2020-09-01 14:46 ` Christoph Hellwig
2020-09-01 14:54 ` Mark Rutland
2020-09-01 15:05 ` Christoph Hellwig
2020-09-01 15:46 ` Christoph Hellwig
2020-09-02 11:43 ` Mark Rutland
2020-09-02 13:32 ` Christoph Hellwig
2020-09-02 17:23 ` Mark Rutland
2020-09-03 6:56 ` Christoph Hellwig
2020-09-04 16:00 ` Mark Rutland
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200831173112.fjfnnup5cfc5t226@treble \
--to=jpoimboe@redhat.com \
--cc=David.Laight@aculab.com \
--cc=aarcange@redhat.com \
--cc=andrew.cooper3@citrix.com \
--cc=dan.j.williams@intel.com \
--cc=hch@lst.de \
--cc=linux-kernel@vger.kernel.org \
--cc=longman@redhat.com \
--cc=luto@kernel.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=viro@zeniv.linux.org.uk \
--cc=will@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).