linux-kernel - Re: [PATCH bpf-next v2 1/7] bpf: Implement bpf_probe_read_kernel

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]

Message-ID: <CAEf4BzYkuPK4_W2DFiyKbwmOrdTJ7GuMLnWSgC8i=tV-FJ8mZg@mail.gmail.com>
Date: Mon, 27 Jan 2025 14:05:45 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Levi Zim <rsworktech@...look.com>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Matt Bobrowski <mattbobrowski@...gle.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Mykola Lysenko <mykolal@...com>, 
	Shuah Khan <shuah@...nel.org>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	linux-trace-kernel <linux-trace-kernel@...r.kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 1/7] bpf: Implement bpf_probe_read_kernel_dynptr
 helper

On Sat, Jan 25, 2025 at 5:05 PM Levi Zim <rsworktech@...look.com> wrote:
>
> On 2025/1/26 00:58, Alexei Starovoitov wrote:
>  > On Sat, Jan 25, 2025 at 12:30 AM Levi Zim via B4 Relay
>  > <devnull+rsworktech.outlook.com@...nel.org> wrote:
>  >> From: Levi Zim <rsworktech@...look.com>
>  >>
>  >> This patch add a helper function bpf_probe_read_kernel_dynptr:
>  >>
>  >> long bpf_probe_read_kernel_dynptr(const struct bpf_dynptr *dst,
>  >>          u32 offset, u32 size, const void *unsafe_ptr, u64 flags);
>  > We stopped adding helpers years ago.
>  > Only new kfuncs are allowed.
>
> Sorry, I didn't know that. Just asking, is there any
> documentation/discussion
> about stopping adding helpers?
>
> I will switch the implementation to kfuncs in v3.

you might want to look at Documentation/bpf/kfuncs.rst as well

>
>  > This particular one doesn't look useful as-is.
>  > The same logic can be expressed with
>  > - create dynptr
>  > - dynptr_slice
>  > - copy_from_kernel
>
> By copy_from_kernel I assume you mean bpf_probe_read_kernel. The problem
> with dynptr_slice_rdwr and probe_read_kernel is that they only support a
> compile-time constant size [1].
>
> But in order to best utilize the space on a BPF ringbuf, it is possible
> to reserve a
> variable length of space as dynptr on a ringbuf with
> bpf_ringbuf_reserve_dynptr.
>
> Then currently we have no way to read a variable length of kernel memory
> into this dynptr, except doing it chunk by chunk[2], which is kinda awkward.
> That's the problem the new helpers trying to solve.
> And I am not the only one needing this kind of feature [3].
>
> Andrii said it would be a straightforward addition as it is a super thin
> wrapper
> around existing functionality (we are just avoiding fixed buffer size
> restrictions of
> existing probe/copy_from APIs)

Yep, exactly. All the probe/copy APIs we have require statically known
dst buffer and, worst still, statically known read size, which makes
it very hard and cumbersome to request data which size is known at
runtime (e.g., process argv/environ data).

So, in short, I think these new dynptr-based APIs are a good addition,
but yeah, they'll have to be added as kfuncs.

One thing that we need to figure out is how to deal with SKB/XDP
dynptrs, we can't just reject them as destination buffers for all
possible APIs. I think we'll need to develop a set of internal helpers
that would handle skb/xdp dynptr cases almost transparently:

  - if skb/xdp memory is linear, we just memcpy/probe_read into it directly;
  - if it's not linear, we use on-the-stack small buffer (128 or 256
bytes, something like that), to do desired operation (memcpy or probe)
into that buffer, and then using generic dynptr write functionality
from that buffer into skb/xdp.

We probably can start simple with excluding xdp/skb, but this should
be the second step.

>
> [1]:
> https://elixir.bootlin.com/linux/v6.12.6/source/kernel/bpf/helpers.c#L2600-L2601
> [2]:
> https://github.com/libbpf/libbpf-bootstrap/commit/046fad60df3e39540937b5ec6ee86054f33d3f28
> [3]: https://github.com/libbpf/libbpf-rs/issues/1041
> [4]:
> https://lore.kernel.org/bpf/CAEf4BzZctXJsR+TwMhmXNWnR0_BV802-3KJw226ZZt8St4xNkw@mail.gmail.com/
>
>  > pw-bot: cr