Messages in this thread | | | From | Jann Horn <> | Date | Mon, 9 Jul 2018 09:41:11 +0200 | 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@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
| |