From: Masami Hiramatsu <mhiramat@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: x86@kernel.org, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
linux-parisc@vger.kernel.org, linux-um@lists.infradead.org,
netdev@vger.kernel.org, bpf@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 11/15] maccess: remove strncpy_from_unsafe
Date: Mon, 11 May 2020 14:34:19 +0900 [thread overview]
Message-ID: <20200511143419.7511026ba60cbf9f6843a153@kernel.org> (raw)
In-Reply-To: <20200506062223.30032-12-hch@lst.de>
On Wed, 6 May 2020 08:22:19 +0200
Christoph Hellwig <hch@lst.de> 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@lst.de>
This looks good to me.
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
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@kernel.org>
next prev parent reply other threads:[~2020-05-11 5:34 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-06 6:22 clean up and streamline probe_kernel_* and friends Christoph Hellwig
2020-05-06 6:22 ` [PATCH 01/15] maccess: unexport probe_kernel_write and probe_user_write Christoph Hellwig
2020-05-06 6:22 ` [PATCH 02/15] maccess: remove various unused weak aliases Christoph Hellwig
2020-05-06 6:22 ` [PATCH 03/15] maccess: remove duplicate kerneldoc commens Christoph Hellwig
2020-05-06 6:22 ` [PATCH 04/15] maccess: clarify kerneldoc comments Christoph Hellwig
2020-05-06 6:22 ` [PATCH 05/15] maccess: update the top of file comment Christoph Hellwig
2020-05-06 6:22 ` [PATCH 06/15] maccess: rename strncpy_from_unsafe_user to strncpy_from_user_unsafe Christoph Hellwig
2020-05-06 6:22 ` [PATCH 07/15] maccess: rename strncpy_from_unsafe_strict to strncpy_from_kernel_unsafe Christoph Hellwig
2020-05-06 6:22 ` [PATCH 08/15] maccess: rename strnlen_unsafe_user to strnlen_user_unsafe Christoph Hellwig
2020-05-06 17:44 ` Linus Torvalds
2020-05-06 17:47 ` Christoph Hellwig
2020-05-06 17:57 ` Linus Torvalds
2020-05-06 6:22 ` [PATCH 09/15] maccess: remove probe_read_common and probe_write_common Christoph Hellwig
2020-05-06 6:22 ` [PATCH 10/15] maccess: unify the probe kernel arch hooks Christoph Hellwig
2020-05-06 6:22 ` [PATCH 11/15] maccess: remove strncpy_from_unsafe Christoph Hellwig
2020-05-11 5:34 ` Masami Hiramatsu [this message]
2020-05-06 6:22 ` [PATCH 12/15] maccess: always use strict semantics for probe_kernel_read Christoph Hellwig
2020-05-11 5:05 ` Masami Hiramatsu
2020-05-11 5:27 ` Masami Hiramatsu
2020-05-06 6:22 ` [PATCH 13/15] maccess: move user access routines together Christoph Hellwig
2020-05-06 6:22 ` [PATCH 14/15] maccess: allow architectures to provide kernel probing directly Christoph Hellwig
2020-05-06 6:22 ` [PATCH 15/15] x86: use non-set_fs based maccess routines Christoph Hellwig
2020-05-06 17:51 ` Linus Torvalds
2020-05-06 18:15 ` Christoph Hellwig
2020-05-06 19:01 ` Linus Torvalds
2020-05-07 5:12 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200511143419.7511026ba60cbf9f6843a153@kernel.org \
--to=mhiramat@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=hch@lst.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-parisc@vger.kernel.org \
--cc=linux-um@lists.infradead.org \
--cc=netdev@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).