On Sat, Dec 2, 2017 at 5:27 AM, Heiko Carstens <[email protected]> wrote: > git commit df04abfd181a ("fs/proc/kcore.c: Add bounce buffer for ktext > data") added a bounce buffer to avoid hardened usercopy > checks. Copying to the bounce buffer was implemented with a simple > memcpy() assuming that it is always valid to read from kernel memory > iff the kern_addr_valid() check passed. > > A simple, but pointless, test case like "dd if=/proc/kcore > of=/dev/null" now can easily crash the kernel, since the former > execption handling on invalid kernel addresses now doesn't work > anymore. > > Also adding a kern_addr_valid() implementation wouldn't help > here. Most architectures simply return 1 here, while a couple > implemented a page table walk to figure out if something is mapped at > the address in question. > With DEBUG_PAGEALLOC active mappings are established and removed all > the time, so that relying on the result of kern_addr_valid() before > executing the memcpy() also doesn't work. > > Therefore simply use probe_kernel_read() to copy to the bounce > buffer. This also allows to simplify read_kcore(). > > At least on s390 this fixes the observed crashes and doesn't introduce > warnings that were removed with df04abfd181a ("fs/proc/kcore.c: Add > bounce buffer for ktext data"), even though the generic > probe_kernel_read() implementation uses uaccess functions. > > While looking into this I'm also wondering if kern_addr_valid() could > be completely removed...(?) > > Fixes: df04abfd181a ("fs/proc/kcore.c: Add bounce buffer for ktext data") > Fixes: f5509cc18daa ("mm: Hardened usercopy") > Signed-off-by: Heiko Carstens <[email protected]>
Thanks for the catch! Yeah, this matches what I just sent to Greg for /dev/mem: https://lkml.org/lkml/2017/12/1/792 Acked-by: Kees Cook <[email protected]> -Kees > --- > fs/proc/kcore.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c > index 4bc85cb8be6a..e8a93bc8285d 100644 > --- a/fs/proc/kcore.c > +++ b/fs/proc/kcore.c > @@ -512,23 +512,15 @@ read_kcore(struct file *file, char __user *buffer, > size_t buflen, loff_t *fpos) > return -EFAULT; > } else { > if (kern_addr_valid(start)) { > - unsigned long n; > - > /* > * Using bounce buffer to bypass the > * hardened user copy kernel text checks. > */ > - memcpy(buf, (char *) start, tsz); > - n = copy_to_user(buffer, buf, tsz); > - /* > - * We cannot distinguish between fault on > source > - * and fault on destination. When this happens > - * we clear too and hope it will trigger the > - * EFAULT again. > - */ > - if (n) { > - if (clear_user(buffer + tsz - n, > - n)) > + if (probe_kernel_read(buf, (void *) start, > tsz)) { > + if (clear_user(buffer, tsz)) > + return -EFAULT; > + } else { > + if (copy_to_user(buffer, buf, tsz)) > return -EFAULT; > } > } else { > -- > 2.13.5 > -- Kees Cook Pixel Security
