[PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Jann Horn]

Don't access the provided buffer out of bounds - this can cause a kernel
out-of-bounds read when invoked through sys_splice() or other things that
use kernel_write()/__kernel_write().

Fixes: 7f8ec5a4f01a ("x86/mtrr: Convert to use strncpy_from_user() helper")
Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/kernel/cpu/mtrr/if.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4021d3859499..40eee6cc4124 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -106,7 +106,8 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 
 	memset(line, 0, LINE_SIZE);
 
-	length = strncpy_from_user(line, buf, LINE_SIZE - 1);
+	len = min_t(size_t, len, LINE_SIZE - 1);
+	length = strncpy_from_user(line, buf, len);
 	if (length < 0)
 		return length;
 
-- 
2.18.0.203.gfac676dfb9-goog

[tip:x86/urgent] x86/mtrr: Don't copy out-of-bounds data in mtrr_write

[tip-bot for Jann Horn]

Commit-ID:  15279df6f26cf2013d713904b4a0c957ae8abb96
Gitweb:     https://git.kernel.org/tip/15279df6f26cf2013d713904b4a0c957ae8abb96
Author:     Jann Horn <jannh@google.com>
AuthorDate: Fri, 6 Jul 2018 23:50:03 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 7 Jul 2018 18:58:41 +0200

x86/mtrr: Don't copy out-of-bounds data in mtrr_write

Don't access the provided buffer out of bounds - this can cause a kernel
out-of-bounds read when invoked through sys_splice() or other things that
use kernel_write()/__kernel_write().

Fixes: 7f8ec5a4f01a ("x86/mtrr: Convert to use strncpy_from_user() helper")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20180706215003.156702-1-jannh@google.com

---
 arch/x86/kernel/cpu/mtrr/if.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/if.c b/arch/x86/kernel/cpu/mtrr/if.c
index 4021d3859499..40eee6cc4124 100644
--- a/arch/x86/kernel/cpu/mtrr/if.c
+++ b/arch/x86/kernel/cpu/mtrr/if.c
@@ -106,7 +106,8 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 
 	memset(line, 0, LINE_SIZE);
 
-	length = strncpy_from_user(line, buf, LINE_SIZE - 1);
+	len = min_t(size_t, len, LINE_SIZE - 1);
+	length = strncpy_from_user(line, buf, len);
 	if (length < 0)
 		return length;
 

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Andy Shevchenko]

On Fri, 2018-07-06 at 23:50 +0200, Jann Horn wrote:
> Don't access the provided buffer out of bounds - this can cause a
> kernel
> out-of-bounds read when invoked through sys_splice() or other things
> that
> use kernel_write()/__kernel_write().
> 

Can you elaborate a bit this change?

Only few places in the kernel do this way and I would like to understand
 why in most of the cases it's okay to supply maximum available length
and here is not the one.


> Fixes: 7f8ec5a4f01a ("x86/mtrr: Convert to use strncpy_from_user()
> helper")
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  arch/x86/kernel/cpu/mtrr/if.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/if.c
> b/arch/x86/kernel/cpu/mtrr/if.c
> index 4021d3859499..40eee6cc4124 100644
> --- a/arch/x86/kernel/cpu/mtrr/if.c
> +++ b/arch/x86/kernel/cpu/mtrr/if.c
> @@ -106,7 +106,8 @@ mtrr_write(struct file *file, const char __user
> *buf, size_t len, loff_t * ppos)
>  
>  	memset(line, 0, LINE_SIZE);
>  
> -	length = strncpy_from_user(line, buf, LINE_SIZE - 1);
> +	len = min_t(size_t, len, LINE_SIZE - 1);
> +	length = strncpy_from_user(line, buf, len);
>  	if (length < 0)
>  		return length;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Jann Horn]

On Mon, Jul 9, 2018 at 8:53 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, 2018-07-06 at 23:50 +0200, Jann Horn wrote:
> > Don't access the provided buffer out of bounds - this can cause a
> > kernel
> > out-of-bounds read when invoked through sys_splice() or other things
> > that
> > use kernel_write()/__kernel_write().
> >
>
> Can you elaborate a bit this change?
>
> Only few places in the kernel do this way and I would like to understand
>  why in most of the cases it's okay to supply maximum available length
> and here is not the one.

In many contexts, it is fine to do something like strncpy_from_user()
with a fixed length without further checks - for example, in normal
syscall handlers, or in ioctl handlers, because invocation of these
implies an intent by the calling code to trigger specifically this
behavior. ->read() and ->write() handlers are special exceptions that
have to adhere to stricter rules because, in essence, reads and writes
on files can be performed by one security context on a file that was
maliciously supplied by another security context. In other words,
invocation of ->read() and ->write() doesn't imply caller intent
beyond "I want to move this many bytes between that file and this
buffer". Specifically, this can happen in two ways:

 - A malicious user can pass an arbitrary file to a setuid binary as
stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
be something normal, like a proper file or a pipe) then calls read(0,
<buf>, <len>), if the kernel disregards the length argument and writes
beyond the end of the buffer, it can corrupt adjacent userspace data,
potentially allowing a user to escalate their privileges; a write
handler is somewhat less interesting because it can probably (as in
this case) only leak out-of-bounds data from the caller, not corrupt
it, but it's still a concern in theory.
 - Almost any ->read() and ->write() handler can be invoked by the
kernel with a buffer argument that points at a *kernel* buffer; when
this happens, *the address limit checks are disabled*, allowing the
->read() or ->write() handler to read and write *kernel memory* using
copy_from_user()/copy_to_user() and other "userspace" accessor
functions. The easiest way to trigger this behavior from userspace is
to use sys_splice().

It's not a big deal in this case because if you can open the mtrr
device, you're probably very highly privileged already, and it's just
a read, not a write, and the data has to adhere to a rather specific
format to be parsed to a point where an attacker could grab the parsed
data - but it's still wrong.

An older mitigation patch for a somewhat similar, but more severe,
problem in another subsystem is the following commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/infiniband?id=e6bd18f57aad1a2d1ef40e646d03ed0f2515c9e3
- Infiniband improperly read pointers to userspace memory in its
->write() handler and then accesses those userspace pointers using
userspace accessor functions. I wrote a PoC back then that could abuse
this to write to an arbitrary kernel address from unprivileged
userspace via sys_splice(), provided that the vulnerable kernel module
was loaded.

> > Fixes: 7f8ec5a4f01a ("x86/mtrr: Convert to use strncpy_from_user()
> > helper")
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  arch/x86/kernel/cpu/mtrr/if.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mtrr/if.c
> > b/arch/x86/kernel/cpu/mtrr/if.c
> > index 4021d3859499..40eee6cc4124 100644
> > --- a/arch/x86/kernel/cpu/mtrr/if.c
> > +++ b/arch/x86/kernel/cpu/mtrr/if.c
> > @@ -106,7 +106,8 @@ mtrr_write(struct file *file, const char __user
> > *buf, size_t len, loff_t * ppos)
> >
> >       memset(line, 0, LINE_SIZE);
> >
> > -     length = strncpy_from_user(line, buf, LINE_SIZE - 1);
> > +     len = min_t(size_t, len, LINE_SIZE - 1);
> > +     length = strncpy_from_user(line, buf, len);
> >       if (length < 0)
> >               return length;
> >
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Andy Shevchenko]

On Mon, 2018-07-09 at 09:41 +0200, Jann Horn wrote:
> On Mon, Jul 9, 2018 at 8:53 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > On Fri, 2018-07-06 at 23:50 +0200, Jann Horn wrote:
> > > Don't access the provided buffer out of bounds - this can cause a
> > > kernel
> > > out-of-bounds read when invoked through sys_splice() or other
> > > things
> > > that
> > > use kernel_write()/__kernel_write().
> > > 
> > 
> > Can you elaborate a bit this change?
> > 
> > Only few places in the kernel do this way and I would like to
> > understand
> >  why in most of the cases it's okay to supply maximum available
> > length
> > and here is not the one.
> 
> In many contexts, it is fine to do something like strncpy_from_user()
> with a fixed length without further checks - for example, in normal
> syscall handlers, or in ioctl handlers, because invocation of these
> implies an intent by the calling code to trigger specifically this
> behavior. ->read() and ->write() handlers are special exceptions that
> have to adhere to stricter rules because, in essence, reads and writes
> on files can be performed by one security context on a file that was
> maliciously supplied by another security context. In other words,
> invocation of ->read() and ->write() doesn't imply caller intent
> beyond "I want to move this many bytes between that file and this
> buffer". Specifically, this can happen in two ways:
> 
>  - A malicious user can pass an arbitrary file to a setuid binary as
> stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
> be something normal, like a proper file or a pipe) then calls read(0,
> <buf>, <len>), if the kernel disregards the length argument and writes
> beyond the end of the buffer, it can corrupt adjacent userspace data,
> potentially allowing a user to escalate their privileges; a write
> handler is somewhat less interesting because it can probably (as in
> this case) only leak out-of-bounds data from the caller, not corrupt
> it, but it's still a concern in theory.
>  - Almost any ->read() and ->write() handler can be invoked by the
> kernel with a buffer argument that points at a *kernel* buffer; when
> this happens, *the address limit checks are disabled*, allowing the
> ->read() or ->write() handler to read and write *kernel memory* using
> copy_from_user()/copy_to_user() and other "userspace" accessor
> functions. The easiest way to trigger this behavior from userspace is
> to use sys_splice().
> 
> It's not a big deal in this case because if you can open the mtrr
> device, you're probably very highly privileged already, and it's just
> a read, not a write, and the data has to adhere to a rather specific
> format to be parsed to a point where an attacker could grab the parsed
> data - but it's still wrong.

Thanks for the above explanation.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Ingo Molnar]

* Jann Horn <jannh@google.com> wrote:

>  - A malicious user can pass an arbitrary file to a setuid binary as
> stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
> be something normal, like a proper file or a pipe) then calls read(0,
> <buf>, <len>), if the kernel disregards the length argument and writes
> beyond the end of the buffer, it can corrupt adjacent userspace data,
> potentially allowing a user to escalate their privileges; a write
> handler is somewhat less interesting because it can probably (as in
> this case) only leak out-of-bounds data from the caller, not corrupt
> it, but it's still a concern in theory.

BTW., a naive question: would it make sense to simply disallow 'special'
fds to be passed to setuid binaries, and fix any user-space that breaks?
(i.e. only allow regular files and pipes/sockets.)

Also, don't allow splice() on special files either, except if the driver 
explicitly opts in to it.

Sounds a lot more robust in the long run than playing whack-a-mole with the 
*inevitable* hole in special read() and write() handlers in our 3,000+ device 
drivers...

Thanks,

	Ingo

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Jann Horn]

+Linus, Andy, Al from the other thread

On Mon, Jul 16, 2018 at 12:03 AM Ingo Molnar <mingo@kernel.org> wrote:
> * Jann Horn <jannh@google.com> wrote:
>
> >  - A malicious user can pass an arbitrary file to a setuid binary as
> > stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
> > be something normal, like a proper file or a pipe) then calls read(0,
> > <buf>, <len>), if the kernel disregards the length argument and writes
> > beyond the end of the buffer, it can corrupt adjacent userspace data,
> > potentially allowing a user to escalate their privileges; a write
> > handler is somewhat less interesting because it can probably (as in
> > this case) only leak out-of-bounds data from the caller, not corrupt
> > it, but it's still a concern in theory.
>
> BTW., a naive question: would it make sense to simply disallow 'special'
> fds to be passed to setuid binaries, and fix any user-space that breaks?
> (i.e. only allow regular files and pipes/sockets.)

If we do that, we'd probably want to do the same for file descriptor
passing through /dev/binder and SCM_RIGHTS. There are already LSM
hooks for most of that because SELinux filters these.

I guess the big question is, how exactly do you decide whether a file
is "special"? Something like a whitelist for files that are not
special based on their filesystem or their file_operations? /dev/null
and /dev/full are character devices, but should probably be things you
can pass to setuid binaries; /dev/fuse is a character device that is
passed over a unix domain socket by the fusermount setuid helper; on
the other hand, the infiniband "rdma_cm" miscdev had a buggy write
handler.
If the necessary whitelist is small enough, it might be a sensible
hardening measure.

> Also, don't allow splice() on special files either, except if the driver
> explicitly opts in to it.

In the thread "[PATCH 24/32] vfs: syscall: Add fsopen() to prepare for
superblock creation [ver #9]", Andy Lutomirski suggested something
similar (https://lore.kernel.org/lkml/338BC3C4-F3E7-48F0-A82E-2C7295B6640E@amacapital.net/):
handler
| (Al- can’t we just stop allowing splice() at all on things that
don’t use iov_iter?)

Linus suggested
(https://lore.kernel.org/lkml/CA+55aFxGqLfu1pjT5421k7KbSd94NWU4fw0H-zJe-qSWwBfAeQ@mail.gmail.com/):

| We could add a FMODE_SPLICE_READ/WRITE bit, and let people opt in to
| splice. We probably should have.

> Sounds a lot more robust in the long run than playing whack-a-mole with the
> *inevitable* hole in special read() and write() handlers in our 3,000+ device
> drivers...

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Linus Torvalds]

On Sun, Jul 15, 2018 at 6:33 PM Jann Horn <jannh@google.com> wrote:
>
> +Linus, Andy, Al from the other thread
>
> On Mon, Jul 16, 2018 at 12:03 AM Ingo Molnar <mingo@kernel.org> wrote:
> >
> > BTW., a naive question: would it make sense to simply disallow 'special'
> > fds to be passed to setuid binaries, and fix any user-space that breaks?
> > (i.e. only allow regular files and pipes/sockets.)
>
> If we do that, we'd probably want to do the same for file descriptor
> passing through /dev/binder and SCM_RIGHTS. There are already LSM
> hooks for most of that because SELinux filters these.

I think it's unlikely to make much sense to try to limit fd passing. I
can't really see any sane model for doing so. By definition, the file
descriptor was opened by the "weak" party (non-suid), and dammit,
that's supposed to be the real permission boundary.

So I don't think "don't pass fd to suid binary" makes any sense at
all. It might break things, and all it does is to paper over the real
issue. It smells like a pointless hack that makes little sense.

The fact that we have had problems in this area is sad, but I'm hoping
people are at least a bit more aware of it.

However:

> > Also, don't allow splice() on special files either, except if the driver
> > explicitly opts in to it.
>
> In the thread "[PATCH 24/32] vfs: syscall: Add fsopen() to prepare for
> superblock creation [ver #9]", Andy Lutomirski suggested something
> similar (https://lore.kernel.org/lkml/338BC3C4-F3E7-48F0-A82E-2C7295B6640E@amacapital.net/):
> handler
> | (Al- can’t we just stop allowing splice() at all on things that
> don’t use iov_iter?)
>
> Linus suggested
> (https://lore.kernel.org/lkml/CA+55aFxGqLfu1pjT5421k7KbSd94NWU4fw0H-zJe-qSWwBfAeQ@mail.gmail.com/):
>
> | We could add a FMODE_SPLICE_READ/WRITE bit, and let people opt in to
> | splice. We probably should have.

Right. We already use FMODE_xyz to limit various operations (ie we can
already say "this file is not seekable" etc), and I have to say, the
special magiuc splice() behavior has been an issue.

We did the "FMODE_PREAD/PWRITE" flags exactly because we wanted to
have the right ESPIPE behavior from file descriptors that simply
couldn't do lseek. And splice() is very much another "some file
descriptors do not like it".

So adding FMODE_SPLICE_READ/WRITE and then just forcing subsystems to
opt in to it sounds fairly simple. The people who really want splice
tend to just want pipes, regular files and network sockets, so
starting out with those opted it should pretty much cover it.

            Linus

Re: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write

[Al Viro]

On Mon, Jul 16, 2018 at 12:03:39AM +0200, Ingo Molnar wrote:
> 
> * Jann Horn <jannh@google.com> wrote:
> 
> >  - A malicious user can pass an arbitrary file to a setuid binary as
> > stdin/stdout/stderr. When the setuid binary (expecting stdin/stdout to
> > be something normal, like a proper file or a pipe) then calls read(0,
> > <buf>, <len>), if the kernel disregards the length argument and writes
> > beyond the end of the buffer, it can corrupt adjacent userspace data,
> > potentially allowing a user to escalate their privileges; a write
> > handler is somewhat less interesting because it can probably (as in
> > this case) only leak out-of-bounds data from the caller, not corrupt
> > it, but it's still a concern in theory.
> 
> BTW., a naive question: would it make sense to simply disallow 'special'
> fds to be passed to setuid binaries, and fix any user-space that breaks?
> (i.e. only allow regular files and pipes/sockets.)

*Ugh*

You do realize that there's a lot of magical mystery shite that looks like
regular files?  And what's wrong with directories, BTW?

While we are at it, "passed" in which sense?  Via SCM_RIGHTS?  Those are
only get accepted if recepient explicitly asks for those - simple read()
or recv() will have them quietly discarded, special or not.

And if it's "inherit over execve()"...  Your definition *will* break.
Instantly.  Right as you try to run su or sudo, with stdin/stdout/stderr
all going to a character device.  Terminal, that is.

Frankly, something like
	copyin_limited(buf, len, kbuf, size)
and several similar helpers would be a lot more productive than open-coding
these checks over and over or trying to come up with definitions of "special"
that would work.

BTW, what's the point open-coding
	if (sscanf(line, "base=%ull size=%ull type=%s", &base, &size, &type) != 3)
		return -EINVAL;
        if ((base & 0xfff) || (size & 0xfff))
                return -EINVAL;
	i = match_string(mtrr_strings, MTRR_NUM_TYPES, type);
        if (i < 0)
                return i;
as
        if (strncmp(line, "base=", 5))
                return -EINVAL;

        base = simple_strtoull(line + 5, &ptr, 0);
        ptr = skip_spaces(ptr);

        if (strncmp(ptr, "size=", 5))
                return -EINVAL;

        size = simple_strtoull(ptr + 5, &ptr, 0);
        if ((base & 0xfff) || (size & 0xfff))
                return -EINVAL;
        ptr = skip_spaces(ptr);

        if (strncmp(ptr, "type=", 5))
                return -EINVAL;
        ptr = skip_spaces(ptr + 5);

        i = match_string(mtrr_strings, MTRR_NUM_TYPES, ptr);
        if (i < 0)
                return i;

Saving the copying of 'type' (which we need since '%n' is declared useless)?