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>
