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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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