lkml.org 
[lkml]   [2018]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] x86/mtrr: don't copy out-of-bounds data in mtrr_write
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

\
 
 \ /
  Last update: 2018-07-09 09:42    [W:0.084 / U:0.116 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site