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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 14 May 2020 23:05:52 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>, 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 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