On Mon, 2018-07-09 at 09:41 +0200, Jann Horn wrote: > On Mon, Jul 9, 2018 at 8:53 AM Andy Shevchenko > <andriy.shevche...@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.shevche...@linux.intel.com> Intel Finland Oy
