| Message ID | 20200521152301.2587579-15-hch@lst.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/23] maccess: unexport probe_kernel_write and probe_user_write | expand |
On Thu, 21 May 2020 17:22:52 +0200 Christoph Hellwig <hch@lst.de> wrote: > Instead of using the dangerous probe_kernel_read and strncpy_from_unsafe > helpers, rework probes to try a user probe based on the address if the > architecture has a common address space for kernel and userspace. > This looks good to me. Acked-by: Masami Hiramatsu <mhiramat@kernel.org> Thank you! > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > kernel/trace/trace_kprobe.c | 72 ++++++++++++++++++++++--------------- > 1 file changed, 43 insertions(+), 29 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 4325f9e7fadaa..4aeaef53ba970 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -1200,6 +1200,15 @@ static const struct file_operations kprobe_profile_ops = { > > /* Kprobe specific fetch functions */ > > +/* Return the length of string -- including null terminal byte */ > +static nokprobe_inline int > +fetch_store_strlen_user(unsigned long addr) > +{ > + const void __user *uaddr = (__force const void __user *)addr; > + > + return strnlen_user_nofault(uaddr, MAX_STRING_SIZE); > +} > + > /* Return the length of string -- including null terminal byte */ > static nokprobe_inline int > fetch_store_strlen(unsigned long addr) > @@ -1207,30 +1216,27 @@ fetch_store_strlen(unsigned long addr) > int ret, len = 0; > u8 c; > > +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > + if (addr < TASK_SIZE) > + return fetch_store_strlen_user(addr); > +#endif > + > do { > - ret = probe_kernel_read(&c, (u8 *)addr + len, 1); > + ret = probe_kernel_read_strict(&c, (u8 *)addr + len, 1); > len++; > } while (c && ret == 0 && len < MAX_STRING_SIZE); > > return (ret < 0) ? ret : len; > } > > -/* Return the length of string -- including null terminal byte */ > -static nokprobe_inline int > -fetch_store_strlen_user(unsigned long addr) > -{ > - const void __user *uaddr = (__force const void __user *)addr; > - > - return strnlen_user_nofault(uaddr, MAX_STRING_SIZE); > -} > - > /* > - * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max > - * length and relative data location. > + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf > + * with max length and relative data location. > */ > static nokprobe_inline int > -fetch_store_string(unsigned long addr, void *dest, void *base) > +fetch_store_string_user(unsigned long addr, void *dest, void *base) > { > + const void __user *uaddr = (__force const void __user *)addr; > int maxlen = get_loc_len(*(u32 *)dest); > void *__dest; > long ret; > @@ -1240,11 +1246,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base) > > __dest = get_loc_data(dest, base); > > - /* > - * Try to get string again, since the string can be changed while > - * probing. > - */ > - ret = strncpy_from_unsafe(__dest, (void *)addr, maxlen); > + ret = strncpy_from_user_nofault(__dest, uaddr, maxlen); > if (ret >= 0) > *(u32 *)dest = make_data_loc(ret, __dest - base); > > @@ -1252,35 +1254,37 @@ fetch_store_string(unsigned long addr, void *dest, void *base) > } > > /* > - * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf > - * with max length and relative data location. > + * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max > + * length and relative data location. > */ > static nokprobe_inline int > -fetch_store_string_user(unsigned long addr, void *dest, void *base) > +fetch_store_string(unsigned long addr, void *dest, void *base) > { > - const void __user *uaddr = (__force const void __user *)addr; > int maxlen = get_loc_len(*(u32 *)dest); > void *__dest; > long ret; > > +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > + if ((unsigned long)addr < TASK_SIZE) > + return fetch_store_string_user(addr, dest, base); > +#endif > + > if (unlikely(!maxlen)) > return -ENOMEM; > > __dest = get_loc_data(dest, base); > > - ret = strncpy_from_user_nofault(__dest, uaddr, maxlen); > + /* > + * Try to get string again, since the string can be changed while > + * probing. > + */ > + ret = strncpy_from_user_nofault(__dest, (void *)addr, maxlen); > if (ret >= 0) > *(u32 *)dest = make_data_loc(ret, __dest - base); > > return ret; > } > > -static nokprobe_inline int > -probe_mem_read(void *dest, void *src, size_t size) > -{ > - return probe_kernel_read(dest, src, size); > -} > - > static nokprobe_inline int > probe_mem_read_user(void *dest, void *src, size_t size) > { > @@ -1289,6 +1293,16 @@ probe_mem_read_user(void *dest, void *src, size_t size) > return probe_user_read(dest, uaddr, size); > } > > +static nokprobe_inline int > +probe_mem_read(void *dest, void *src, size_t size) > +{ > +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > + if ((unsigned long)src < TASK_SIZE) > + return probe_mem_read_user(dest, src, size); > +#endif > + return probe_kernel_read_strict(dest, src, size); > +} > + > /* Note that we don't verify it, since the code does not come from user space */ > static int > process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest, > -- > 2.26.2 >
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 4325f9e7fadaa..4aeaef53ba970 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -1200,6 +1200,15 @@ static const struct file_operations kprobe_profile_ops = { /* Kprobe specific fetch functions */ +/* Return the length of string -- including null terminal byte */ +static nokprobe_inline int +fetch_store_strlen_user(unsigned long addr) +{ + const void __user *uaddr = (__force const void __user *)addr; + + return strnlen_user_nofault(uaddr, MAX_STRING_SIZE); +} + /* Return the length of string -- including null terminal byte */ static nokprobe_inline int fetch_store_strlen(unsigned long addr) @@ -1207,30 +1216,27 @@ fetch_store_strlen(unsigned long addr) int ret, len = 0; u8 c; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if (addr < TASK_SIZE) + return fetch_store_strlen_user(addr); +#endif + do { - ret = probe_kernel_read(&c, (u8 *)addr + len, 1); + ret = probe_kernel_read_strict(&c, (u8 *)addr + len, 1); len++; } while (c && ret == 0 && len < MAX_STRING_SIZE); return (ret < 0) ? ret : len; } -/* Return the length of string -- including null terminal byte */ -static nokprobe_inline int -fetch_store_strlen_user(unsigned long addr) -{ - const void __user *uaddr = (__force const void __user *)addr; - - return strnlen_user_nofault(uaddr, MAX_STRING_SIZE); -} - /* - * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max - * length and relative data location. + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf + * with max length and relative data location. */ static nokprobe_inline int -fetch_store_string(unsigned long addr, void *dest, void *base) +fetch_store_string_user(unsigned long addr, void *dest, void *base) { + const void __user *uaddr = (__force const void __user *)addr; int maxlen = get_loc_len(*(u32 *)dest); void *__dest; long ret; @@ -1240,11 +1246,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base) __dest = get_loc_data(dest, base); - /* - * Try to get string again, since the string can be changed while - * probing. - */ - ret = strncpy_from_unsafe(__dest, (void *)addr, maxlen); + ret = strncpy_from_user_nofault(__dest, uaddr, maxlen); if (ret >= 0) *(u32 *)dest = make_data_loc(ret, __dest - base); @@ -1252,35 +1254,37 @@ fetch_store_string(unsigned long addr, void *dest, void *base) } /* - * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf - * with max length and relative data location. + * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max + * length and relative data location. */ static nokprobe_inline int -fetch_store_string_user(unsigned long addr, void *dest, void *base) +fetch_store_string(unsigned long addr, void *dest, void *base) { - const void __user *uaddr = (__force const void __user *)addr; int maxlen = get_loc_len(*(u32 *)dest); void *__dest; long ret; +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)addr < TASK_SIZE) + return fetch_store_string_user(addr, dest, base); +#endif + if (unlikely(!maxlen)) return -ENOMEM; __dest = get_loc_data(dest, base); - ret = strncpy_from_user_nofault(__dest, uaddr, maxlen); + /* + * Try to get string again, since the string can be changed while + * probing. + */ + ret = strncpy_from_user_nofault(__dest, (void *)addr, maxlen); if (ret >= 0) *(u32 *)dest = make_data_loc(ret, __dest - base); return ret; } -static nokprobe_inline int -probe_mem_read(void *dest, void *src, size_t size) -{ - return probe_kernel_read(dest, src, size); -} - static nokprobe_inline int probe_mem_read_user(void *dest, void *src, size_t size) { @@ -1289,6 +1293,16 @@ probe_mem_read_user(void *dest, void *src, size_t size) return probe_user_read(dest, uaddr, size); } +static nokprobe_inline int +probe_mem_read(void *dest, void *src, size_t size) +{ +#ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE + if ((unsigned long)src < TASK_SIZE) + return probe_mem_read_user(dest, src, size); +#endif + return probe_kernel_read_strict(dest, src, size); +} + /* Note that we don't verify it, since the code does not come from user space */ static int process_fetch_insn(struct fetch_insn *code, struct pt_regs *regs, void *dest,
Instead of using the dangerous probe_kernel_read and strncpy_from_unsafe helpers, rework probes to try a user probe based on the address if the architecture has a common address space for kernel and userspace. Signed-off-by: Christoph Hellwig <hch@lst.de> --- kernel/trace/trace_kprobe.c | 72 ++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 29 deletions(-)