From e1df462cf02fafbce440755f69ca595192c653a2 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 8 Jun 2020 21:34:50 -0700 Subject: [PATCH] BACKPORT: maccess: always use strict semantics for probe_kernel_read Except for historical confusion in the kprobes/uprobes and bpf tracers, which has been fixed now, there is no good reason to ever allow user memory accesses from probe_kernel_read. Switch probe_kernel_read to only read from kernel memory. [akpm@linux-foundation.org: update it for "mm, dump_page(): do not crash with invalid mapping pointer"] Change-Id: I91f7ef83a3de5dde24787608ae2cbd2770de10ac Signed-off-by: Christoph Hellwig Signed-off-by: Andrew Morton Cc: Alexei Starovoitov Cc: Daniel Borkmann Cc: "H. Peter Anvin" Cc: Ingo Molnar Cc: Masami Hiramatsu Cc: Thomas Gleixner Link: http://lkml.kernel.org/r/20200521152301.2587579-17-hch@lst.de Signed-off-by: Linus Torvalds --- arch/parisc/lib/memcpy.c | 2 +- arch/um/kernel/maccess.c | 2 +- arch/x86/mm/maccess.c | 9 ++------- include/linux/uaccess.h | 4 +--- kernel/trace/bpf_trace.c | 2 +- mm/maccess.c | 39 ++++++--------------------------------- 6 files changed, 12 insertions(+), 46 deletions(-) diff --git a/arch/parisc/lib/memcpy.c b/arch/parisc/lib/memcpy.c index 064ecdb511ab..da2354f10f18 100644 --- a/arch/parisc/lib/memcpy.c +++ b/arch/parisc/lib/memcpy.c @@ -71,7 +71,7 @@ void * memcpy(void * dst,const void *src, size_t count) EXPORT_SYMBOL(raw_copy_in_user); EXPORT_SYMBOL(memcpy); -bool probe_kernel_read_allowed(const void *unsafe_src, size_t size, bool strict) +bool probe_kernel_read_allowed(const void *unsafe_src, size_t size) { if ((unsigned long)unsafe_src < PAGE_SIZE) return false; diff --git a/arch/um/kernel/maccess.c b/arch/um/kernel/maccess.c index 364a5285b5b6..b90b95eb75b0 100644 --- a/arch/um/kernel/maccess.c +++ b/arch/um/kernel/maccess.c @@ -10,7 +10,7 @@ #include #include -bool probe_kernel_read_allowed(const void *src, size_t size, bool strict) +bool probe_kernel_read_allowed(const void *src, size_t size) { void *psrc = (void *)rounddown((unsigned long)src, PAGE_SIZE); diff --git a/arch/x86/mm/maccess.c b/arch/x86/mm/maccess.c index 86000c1150e8..e1d7d7477c22 100644 --- a/arch/x86/mm/maccess.c +++ b/arch/x86/mm/maccess.c @@ -9,13 +9,10 @@ static __always_inline u64 canonical_address(u64 vaddr, u8 vaddr_bits) return ((s64)vaddr << (64 - vaddr_bits)) >> (64 - vaddr_bits); } -bool probe_kernel_read_allowed(const void *unsafe_src, size_t size, bool strict) +bool probe_kernel_read_allowed(const void *unsafe_src, size_t size) { unsigned long vaddr = (unsigned long)unsafe_src; - if (!strict) - return true; - /* * Range covering the highest possible canonical userspace address * as well as non-canonical address range. For the canonical range @@ -25,10 +22,8 @@ bool probe_kernel_read_allowed(const void *unsafe_src, size_t size, bool strict) canonical_address(vaddr, boot_cpu_data.x86_virt_bits) == vaddr; } #else -bool probe_kernel_read_allowed(const void *unsafe_src, size_t size, bool strict) +bool probe_kernel_read_allowed(const void *unsafe_src, size_t size) { - if (!strict) - return true; return (unsigned long)unsafe_src >= TASK_SIZE_MAX; } #endif diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 30b19fb520a8..c3d2520f9a69 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -300,11 +300,9 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src, return 0; } -bool probe_kernel_read_allowed(const void *unsafe_src, size_t size, - bool strict); +bool probe_kernel_read_allowed(const void *unsafe_src, size_t size); extern long probe_kernel_read(void *dst, const void *src, size_t size); -extern long probe_kernel_read_strict(void *dst, const void *src, size_t size); extern long probe_user_read(void *dst, const void __user *src, size_t size); extern long notrace probe_kernel_write(void *dst, const void *src, size_t size); diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index ac5076403c84..6b0dfb035fc8 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -213,7 +213,7 @@ bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr) { int ret; - ret = probe_kernel_read_strict(dst, unsafe_ptr, size); + ret = probe_kernel_read(dst, unsafe_ptr, size); if (unlikely(ret < 0)) memset(dst, 0, size); return ret; diff --git a/mm/maccess.c b/mm/maccess.c index d7db69dde1b8..4fb1e9cfc851 100644 --- a/mm/maccess.c +++ b/mm/maccess.c @@ -5,38 +5,16 @@ #include #include -static long __probe_kernel_read(void *dst, const void *src, size_t size, - bool strict); static long __strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count, bool strict); -bool __weak probe_kernel_read_allowed(const void *unsafe_src, size_t size, - bool strict) +bool __weak probe_kernel_read_allowed(const void *unsafe_src, size_t size) { return true; } /** - * probe_kernel_read(): safely attempt to read from a kernel-space location - * @dst: pointer to the buffer that shall take the data - * @src: address to read from - * @size: size of the data chunk - * - * Same as probe_kernel_read_strict() except that for architectures with - * not fully separated user and kernel address spaces this function also works - * for user address tanges. - * - * DO NOT USE THIS FUNCTION - it is broken on architectures with entirely - * separate kernel and user address spaces, and also a bad idea otherwise. - */ -long probe_kernel_read(void *dst, const void *src, size_t size) -{ - return __probe_kernel_read(dst, src, size, false); -} -EXPORT_SYMBOL_GPL(probe_kernel_read); - -/** - * probe_kernel_read_strict(): safely attempt to read from kernel-space + * probe_kernel_read(): safely attempt to read from kernel-space * @dst: pointer to the buffer that shall take the data * @src: address to read from * @size: size of the data chunk @@ -55,18 +33,12 @@ EXPORT_SYMBOL_GPL(probe_kernel_read); * probing memory on a user address range where probe_user_read() is supposed * to be used instead. */ -long probe_kernel_read_strict(void *dst, const void *src, size_t size) -{ - return __probe_kernel_read(dst, src, size, true); -} - -static long __probe_kernel_read(void *dst, const void *src, size_t size, - bool strict) +long probe_kernel_read(void *dst, const void *src, size_t size) { long ret; mm_segment_t old_fs = get_fs(); - if (!probe_kernel_read_allowed(src, size, strict)) + if (!probe_kernel_read_allowed(src, size)) return -EFAULT; set_fs(KERNEL_DS); @@ -80,6 +52,7 @@ static long __probe_kernel_read(void *dst, const void *src, size_t size, return -EFAULT; return 0; } +EXPORT_SYMBOL_GPL(probe_kernel_read); /** * probe_user_read(): safely attempt to read from a user-space location @@ -224,7 +197,7 @@ static long __strncpy_from_unsafe(char *dst, const void *unsafe_addr, if (unlikely(count <= 0)) return 0; - if (!probe_kernel_read_allowed(unsafe_addr, count, strict)) + if (!probe_kernel_read_allowed(unsafe_addr, count)) return -EFAULT; set_fs(KERNEL_DS);