linux-kernel - Re: [PATCH] bpf: verifier: allow arrays of progs to be used in sleepable context

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: Wed, 24 Apr 2024 16:16:56 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: Alexei Starovoitov <ast@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, 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>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bpf: verifier: allow arrays of progs to be used in
 sleepable context

On Apr 22 2024, Benjamin Tissoires wrote:
> On Apr 22 2024, Daniel Borkmann wrote:
> > On 4/22/24 9:16 AM, Benjamin Tissoires wrote:
> > > Arrays of progs are underlying using regular arrays, but they can only
> > > be updated from a syscall.
> > > Therefore, they should be safe to use while in a sleepable context.
> > > 
> > > This is required to be able to call bpf_tail_call() from a sleepable
> > > tracing bpf program.
> > > 
> > > Signed-off-by: Benjamin Tissoires <bentiss@...nel.org>
> > > ---
> > > Hi,
> > > 
> > > a small patch to allow to have:
> > > 
> > > ```
> > > SEC("fmod_ret.s/__hid_bpf_tail_call_sleepable")
> > > int BPF_PROG(hid_tail_call_sleepable, struct hid_bpf_ctx *hctx)
> > > {
> > > 	bpf_tail_call(ctx, &hid_jmp_table, hctx->index);
> > > 
> > > 	return 0;
> > > }
> > > ```
> > > 
> > > This should allow me to add bpf hooks to functions that communicate with
> > > the hardware.
> > 
> > Could you also add selftests to it? In particular, I'm thinking that this is not
> > sufficient given also bpf_prog_map_compatible() needs to be extended to check on
> > prog->sleepable. For example we would need to disallow calling sleepable programs
> > in that map from non-sleepable context.
> 
> Just to be sure, if I have to change bpf_prog_map_compatible(), that
> means that a prog array map can only have sleepable or non-sleepable
> programs, but not both at the same time?
> 
> FWIW, indeed, I just tested and the BPF verifier/core is happy with this
> patch only if the bpf_tail_call is issued from a non-sleepable context
> (and crashes as expected).
> 
> But that seems to be a different issue TBH: I can store a sleepable BPF
> program in a prog array and run it from a non sleepable context. I don't
> need the patch at all as bpf_tail_call() is normally declared. I assume
> your suggestion to change bpf_prog_map_compatible() will fix that part.
> 
> I'll digg some more tomorrow.
> 

Quick update:
forcing the prog array to only contain sleepable programs or not seems
to do the trick, but I'm down a rabbit hole as when I return from my
trampoline, I get an invalid page fault, trying to execute NX-protected
page.

I'll report if it's because of HID-BPF or if there are more work to be
doing for bpf_tail_call (which I suspect).

Cheers,
Benjamin