linux-kernel - Re: [PATCH bpf-next 11/11] bpf: Fall back to nospec for spec path verification

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: <CAADnVQKL-NwxigMWM+U=n5ZXPG+xHYzSTEv0Rq8Y91m45eRJDw@mail.gmail.com>
Date: Tue, 18 Mar 2025 19:40:44 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Luis Gerhorst <luis.gerhorst@....de>
Cc: 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>, Puranjay Mohan <puranjay@...nel.org>, 
	Xu Kuohai <xukuohai@...weicloud.com>, Catalin Marinas <catalin.marinas@....com>, 
	Will Deacon <will@...nel.org>, Hari Bathini <hbathini@...ux.ibm.com>, 
	Christophe Leroy <christophe.leroy@...roup.eu>, Naveen N Rao <naveen@...nel.org>, 
	Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman <mpe@...erman.id.au>, 
	Nicholas Piggin <npiggin@...il.com>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, 
	Henriette Herzog <henriette.herzog@....de>, Cupertino Miranda <cupertino.miranda@...cle.com>, 
	Matan Shachnai <m.shachnai@...il.com>, Dimitar Kanaliev <dimitar.kanaliev@...eground.com>, 
	Shung-Hsi Yu <shung-hsi.yu@...e.com>, Daniel Xu <dxu@...uu.xyz>, bpf <bpf@...r.kernel.org>, 
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, LKML <linux-kernel@...r.kernel.org>, 
	ppc-dev <linuxppc-dev@...ts.ozlabs.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>, George Guo <guodongtai@...inos.cn>, 
	WANG Xuerui <git@...0n.name>, Tiezhu Yang <yangtiezhu@...ngson.cn>, Maximilian Ott <ott@...fau.de>, 
	Milan Stephan <milan.stephan@....de>
Subject: Re: [PATCH bpf-next 11/11] bpf: Fall back to nospec for spec path verification

On Thu, Mar 13, 2025 at 10:57 AM Luis Gerhorst <luis.gerhorst@....de> wrote:
>
> This trades verification complexity for runtime overheads due to the
> nospec inserted because of the EINVAL.
>
> With increased limits this allows applying mitigations to large BPF
> progs such as the Parca Continuous Profiler's prog. However, this
> requires a jump-seq limit of 256k. In any case, the same principle
> should apply to smaller programs therefore include it even if the limit
> stays at 8k for now. Most programs in [1] only require a limit of 32k.

Do you mean that without this change the verifier needs 256k
jmp limit to load Parca's prog as unpriv due to speculative
path exploration with push_stack ?

And this change uses 4k as a trade-off between prog runtime
and verification time ?

But tracing progs use bpf_probe_read_kernel(), so they're never going
to be unpriv.

> @@ -2010,6 +2011,19 @@ static struct bpf_verifier_state *push_stack(struct bpf_verifier_env *env,
>         struct bpf_verifier_stack_elem *elem;
>         int err;
>
> +       if (!env->bypass_spec_v1 &&
> +           cur->speculative &&

Should this be
(cur->speculative || speculative)
?

In general I'm not convinced that the approach is safe.

This recoverable EINVAL means that exploration under speculation
stops early, but there could be more branches and they won't be
sanitized with extra lfence.
So speculative execution can still happen at later insns.

Similar concern in patch 7:
+ if (state->speculative && cur_aux(env)->nospec)
+   goto process_bpf_exit;

One lfence at this insn doesn't stop speculation until the program end.
Only at this insn. The rest of the code is free to speculate.

The refactoring in patches 1-3 is nice.
Patches 4-5 are tricky and somewhat questionable, but make sense.
Patch 7 without early goto process_bpf_exit looks correct too,
Patch 8 is borderline. Feels like it's opening the door for
new vulnerabilities and space to explore for security researchers.
We disabled unpriv bpf by default and have no intentions to enable it.
Even if we land the whole thing the unpriv will stay disabled.
So trade offs don't appear favorable.