Message-ID: <CAG48ez105Jan3BDAKssThnbNcr3yBMFTbdQHEHwXdmZUvE5VQw@mail.gmail.com>
Date:   Mon, 9 Jul 2018 09:41:11 +0200
From:   Jann Horn <jannh@...gle.com>
To:     andriy.shevchenko@...ux.intel.com
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [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@...ux.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@...gle.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@...ux.intel.com>
> Intel Finland Oy

Powered by blists - more mailing lists