Re: [PATCH bpf 3/3] bpf: restrict bpf_trace_printk()'s %s usage and add %psK, %psU specifier

[Yonghong Song <yhs@fb.com>]

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@linux-foundation.org>
> Reported-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
> Cc: Christoph Hellwig <hch@lst.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
>   ---------------
>   
[...]