On Wed, 6 May 2020 08:22:19 +0200 Christoph Hellwig <hch@xxxxxx> wrote: > All three callers really should try the explicit kernel and user > copies instead. One has already deprecated the somewhat dangerous > either kernel or user address concept, the other two still need to > follow up eventually. > > Signed-off-by: Christoph Hellwig <hch@xxxxxx> This looks good to me. Reviewed-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx> Thank you, > --- > include/linux/uaccess.h | 1 - > kernel/trace/bpf_trace.c | 40 ++++++++++++++++++++++++++----------- > kernel/trace/trace_kprobe.c | 5 ++++- > mm/maccess.c | 39 +----------------------------------- > 4 files changed, 33 insertions(+), 52 deletions(-) > > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h > index f8c47395a92df..09d6e358883cc 100644 > --- a/include/linux/uaccess.h > +++ b/include/linux/uaccess.h > @@ -311,7 +311,6 @@ 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); > extern long notrace probe_user_write(void __user *dst, const void *src, size_t size); > > -extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count); > extern long strncpy_from_kernel_unsafe(char *dst, const void *unsafe_addr, > long count); > extern long strncpy_from_user_unsafe(char *dst, const void __user *unsafe_addr, > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index e4e202f433903..ffe841433caa1 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -229,9 +229,10 @@ bpf_probe_read_kernel_str_common(void *dst, u32 size, const void *unsafe_ptr, > int ret = security_locked_down(LOCKDOWN_BPF_READ); > > if (unlikely(ret < 0)) > - goto out; > + goto fail; > + > /* > - * The strncpy_from_unsafe_*() call will likely not fill the entire > + * The strncpy_from_*_unsafe() call will likely not fill the entire > * buffer, but that's okay in this circumstance as we're probing > * arbitrary memory anyway similar to bpf_probe_read_*() and might > * as well probe the stack. Thus, memory is explicitly cleared > @@ -239,11 +240,18 @@ bpf_probe_read_kernel_str_common(void *dst, u32 size, const void *unsafe_ptr, > * code altogether don't copy garbage; otherwise length of string > * is returned that can be used for bpf_perf_event_output() et al. > */ > - ret = compat ? strncpy_from_unsafe(dst, unsafe_ptr, size) : > - strncpy_from_kernel_unsafe(dst, unsafe_ptr, size); > - if (unlikely(ret < 0)) > -out: > - memset(dst, 0, size); > + ret = strncpy_from_kernel_unsafe(dst, unsafe_ptr, size); > + if (unlikely(ret < 0)) { > + if (compat) > + ret = strncpy_from_user_unsafe(dst, > + (__force const void __user *)unsafe_ptr, > + size); > + if (ret < 0) > + goto fail; > + } > + return 0; > +fail: > + memset(dst, 0, size); > return ret; > } > > @@ -321,6 +329,17 @@ static const struct bpf_func_proto *bpf_get_probe_write_proto(void) > return &bpf_probe_write_user_proto; > } > > +#define BPF_STRNCPY_LEN 64 > + > +static void bpf_strncpy(char *buf, long unsafe_addr) > +{ > + buf[0] = 0; > + if (strncpy_from_kernel_unsafe(buf, (void *)unsafe_addr, > + BPF_STRNCPY_LEN)) > + strncpy_from_user_unsafe(buf, (void __user *)unsafe_addr, > + BPF_STRNCPY_LEN); > +} > + > /* > * Only limited trace_printk() conversion specifiers allowed: > * %d %i %u %x %ld %li %lu %lx %lld %lli %llu %llx %p %s > @@ -332,7 +351,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, > int mod[3] = {}; > int fmt_cnt = 0; > u64 unsafe_addr; > - char buf[64]; > + char buf[BPF_STRNCPY_LEN]; > int i; > > /* > @@ -387,10 +406,7 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, > arg3 = (long) buf; > break; > } > - buf[0] = 0; > - strncpy_from_unsafe(buf, > - (void *) (long) unsafe_addr, > - sizeof(buf)); > + bpf_strncpy(buf, unsafe_addr); > } > continue; > } > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index a7f43c7ec9880..525d12137325c 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -1238,7 +1238,10 @@ fetch_store_string(unsigned long addr, void *dest, void *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_kernel_unsafe(__dest, (void *)addr, maxlen); > + if (ret < 0) > + ret = strncpy_from_user_unsafe(__dest, (void __user *)addr, > + maxlen); > if (ret >= 0) > *(u32 *)dest = make_data_loc(ret, __dest - base); > > diff --git a/mm/maccess.c b/mm/maccess.c > index 11563129cd490..cbd9d668aa46e 100644 > --- a/mm/maccess.c > +++ b/mm/maccess.c > @@ -8,8 +8,6 @@ > > 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(void *dst, const void *unsafe_src, > size_t size, bool strict) > @@ -156,35 +154,6 @@ long probe_user_write(void __user *dst, const void *src, size_t size) > return 0; > } > > -/** > - * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address. > - * @dst: Destination address, in kernel space. This buffer must be at > - * least @count bytes long. > - * @unsafe_addr: Unsafe address. > - * @count: Maximum number of bytes to copy, including the trailing NUL. > - * > - * Copies a NUL-terminated string from unsafe address to kernel buffer. > - * > - * On success, returns the length of the string INCLUDING the trailing NUL. > - * > - * If access fails, returns -EFAULT (some data may have been copied > - * and the trailing NUL added). > - * > - * If @count is smaller than the length of the string, copies @count-1 bytes, > - * sets the last byte of @dst buffer to NUL and returns @count. > - * > - * Same as strncpy_from_kernel_unsafe() 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 strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count) > -{ > - return __strncpy_from_unsafe(dst, unsafe_addr, count, false); > -} > - > /** > * strncpy_from_kernel_unsafe: - Copy a NUL terminated string from unsafe > * address. > @@ -204,12 +173,6 @@ long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count) > * sets the last byte of @dst buffer to NUL and returns @count. > */ > long strncpy_from_kernel_unsafe(char *dst, const void *unsafe_addr, long count) > -{ > - return __strncpy_from_unsafe(dst, unsafe_addr, count, true); > -} > - > -static long __strncpy_from_unsafe(char *dst, const void *unsafe_addr, > - long count, bool strict) > { > mm_segment_t old_fs = get_fs(); > const void *src = unsafe_addr; > @@ -217,7 +180,7 @@ static long __strncpy_from_unsafe(char *dst, const void *unsafe_addr, > > if (unlikely(count <= 0)) > return 0; > - if (!probe_kernel_read_allowed(dst, unsafe_addr, count, strict)) > + if (!probe_kernel_read_allowed(dst, unsafe_addr, count, true)) > return -EFAULT; > > set_fs(KERNEL_DS); > -- > 2.26.2 > -- Masami Hiramatsu <mhiramat@xxxxxxxxxx>