[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <34e9da6e-1f1e-30c2-5863-55f7d8506eb8@fb.com>
Date: Thu, 14 May 2020 11:10:32 -0700
From: Yonghong Song <yhs@...com>
To: Daniel Borkmann <daniel@...earbox.net>, <ast@...nel.org>
CC: <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
<torvalds@...ux-foundation.org>, <mhiramat@...nel.org>,
<brendan.d.gregg@...il.com>, <hch@....de>,
<john.fastabend@...il.com>
Subject: Re: [PATCH bpf 3/3] bpf: restrict bpf_trace_printk()'s %s usage and
add %psK, %psU specifier
On 5/14/20 9:16 AM, Daniel Borkmann wrote:
> Usage of plain %s conversion specifier in bpf_trace_printk() suffers from the
> very same issue as bpf_probe_read{,str}() helpers, that is, it is broken on
> archs with overlapping address ranges.
>
> While the helpers have been addressed through work in 6ae08ae3dea2 ("bpf: Add
> probe_read_{user, kernel} and probe_read_{user, kernel}_str helpers"), we need
> an option for bpf_trace_printk() as well to fix it.
>
> Similarly as with the helpers, force users to make an explicit choice by adding
> %psK and %psU specifier to bpf_trace_printk() which will then pick the corresponding
> strncpy_from_unsafe*() variant to perform the access under KERNEL_DS or USER_DS.
In bpf_trace_printk(), we only print strings.
In bpf-next bpf_iter bpf_seq_printf() helper, introduced by
commit 492e639f0c22 ("bpf: Add bpf_seq_printf and bpf_seq_write
helpers"), print strings and ip addresses %p{i,I}{4,6}.
Alan in
https://lore.kernel.org/bpf/alpine.LRH.2.21.2005141738050.23867@localhost/T
proposed BTF based type printing with a new format specifier
%pT, which potentially will be used in bpf_trace_printk() and
bpf_seq_printf().
In the future, we may want to support more %p<...> format in these
helpers. I am wondering whether we can have generic way so we only need
to change lib/vsprintf.c once.
Maybe using %pk<...> to specify the kernel address and %pu<...> to
specify user address space. In the above example, we will have
%pks, %pus, %pki4 or %pui4, etc. Does this make sense?
>
> Existing %s for legacy users is still kept working for archs where it is not
> broken and therefore gated through CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE.
>
> Fixes: 8d3b7dce8622 ("bpf: add support for %s specifier to bpf_trace_printk()")
> Reported-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Reported-by: Christoph Hellwig <hch@....de>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Brendan Gregg <brendan.d.gregg@...il.com>
> Cc: Christoph Hellwig <hch@....de>
> ---
> Documentation/core-api/printk-formats.rst | 14 ++++
> kernel/trace/bpf_trace.c | 92 +++++++++++++++--------
> lib/vsprintf.c | 7 +-
> 3 files changed, 81 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 8ebe46b1af39..76b5f4f265cb 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -112,6 +112,20 @@ used when printing stack backtraces. The specifier takes into
> consideration the effect of compiler optimisations which may occur
> when tail-calls are used and marked with the noreturn GCC attribute.
>
> +Probed Strings from BPF
> +-----------------------
> +
> +::
> +
> + %psK kernel_string
> + %psU user_string
> +
> +The ``sK`` and ``sU`` specifiers are used for printing a string from probed
> +memory. From regular vsnprintf(), they are equivalent to ``%s``, however,
> +when used out of BPF's bpf_trace_printk() it reads a string of up to 64 bytes
> +in memory without faulting. For ``K`` specifier, the string is probed out of
> +kernel memory whereas for ``U`` specifier, it is probed out of user memory.
> +
> Kernel Pointers
> ---------------
>
[...]
Powered by blists - more mailing lists