On 5/14/20 8:10 PM, Yonghong Song wrote:
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.
Right ...
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}.
... and here only kernel buffers.
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?
Ah, right, once bpf merges back into bpf-next, we should consolidate these. I didn't want
to add the strncpy_from_unsafe*() right into lib/vsprintf.c since then we'd open it up to
all possible call-sites whereas it's really just needed out of bpf_trace_printk() for the
fix. I think it probably might make sense to add a generic lightweight layer to consolidate
all the bpf-related printk handling where we can statically specify a config e.g. as flags
of allowed specifiers which then internally takes care of checking the fmt specifiers for
sanity and does the probe read handling before passing down into lower layers. Thinking of
the case of adding %p{i,I}{4,6} to bpf_trace_printk(), for example, and assuming we'd had
a case where it needs to be probed out of both, then %pk<...> and %pu<...> modifier feels
reasonable indeed. I'll do a v2 to implement %pks and %pus instead.
Thanks,
Daniel
