public inbox for bpf@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next 1/4] bpf: Propagate error from visit_tailcall_insn
@ 2026-04-08 15:37 Daniel Borkmann
  2026-04-08 15:37 ` [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs Daniel Borkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 15:37 UTC (permalink / raw)
  To: bpf; +Cc: ast, eddyz87, info

Commit e40f5a6bf88a ("bpf: correct stack liveness for tail calls") added
visit_tailcall_insn() but did not check its return value.

Fixes: e40f5a6bf88a ("bpf: correct stack liveness for tail calls")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 594260c1f382..db009d509ade 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19355,8 +19355,11 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 				mark_subprog_might_sleep(env, t);
 			if (bpf_helper_changes_pkt_data(insn->imm))
 				mark_subprog_changes_pkt_data(env, t);
-			if (insn->imm == BPF_FUNC_tail_call)
-				visit_tailcall_insn(env, t);
+			if (insn->imm == BPF_FUNC_tail_call) {
+				ret = visit_tailcall_insn(env, t);
+				if (ret)
+					return ret;
+			}
 		} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
 			struct bpf_kfunc_call_arg_meta meta;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs
  2026-04-08 15:37 [PATCH bpf-next 1/4] bpf: Propagate error from visit_tailcall_insn Daniel Borkmann
@ 2026-04-08 15:37 ` Daniel Borkmann
  2026-04-08 16:20   ` Alexei Starovoitov
  2026-04-08 15:37 ` [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer Daniel Borkmann
  2026-04-08 15:37 ` [PATCH bpf-next 4/4] selftests/bpf: Add tests for ld_{abs,ind} failure path in subprogs Daniel Borkmann
  2 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 15:37 UTC (permalink / raw)
  To: bpf; +Cc: ast, eddyz87, info

Usage of ld_{abs,ind} instructions got extended into subprogs some time
ago via commit 09b28d76eac4 ("bpf: Add abnormal return checks."). These
are only allowed in subprograms when the latter are BTF annotated and
have scalar return types.

The code generator in bpf_gen_ld_abs() has an abnormal exit path (r0=0 +
exit) from legacy cBPF times. While the enforcement is on scalar return
types, the verifier must also simulate the path of abnormal exit if the
packet data load via ld_{abs,ind} failed.

This is currently not the case. Fix it by having the verifier simulate
both success and failure paths, and extend it in similar ways as we do
for tail calls. The success path (r0=unknown, continue to next insn) is
pushed onto stack for later validation and the r0=0 and return to the
caller is done on the fall-through side.

Fixes: 09b28d76eac4 ("bpf: Add abnormal return checks.")
Reported-by: STAR Labs SG <info@starlabs.sg>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index db009d509ade..46f55ea6684e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18357,6 +18357,22 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 	mark_reg_unknown(env, regs, BPF_REG_0);
 	/* ld_abs load up to 32-bit skb data. */
 	regs[BPF_REG_0].subreg_def = env->insn_idx + 1;
+	/*
+	 * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0
+	 * which must be explored by the verifier when in a subprog.
+	 */
+	if (env->cur_state->curframe) {
+		struct bpf_verifier_state *branch;
+
+		branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
+		if (IS_ERR(branch))
+			return PTR_ERR(branch);
+		mark_reg_known_zero(env, regs, BPF_REG_0);
+		err = prepare_func_exit(env, &env->insn_idx);
+		if (err)
+			return err;
+		env->insn_idx--;
+	}
 	return 0;
 }
 
@@ -19276,7 +19292,12 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env)
 	return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING;
 }
 
-static int visit_tailcall_insn(struct bpf_verifier_env *env, int t)
+/*
+ * Instructions that can abnormally return from a subprog (tail_call
+ * upon success, ld_{abs,ind} upon load failure) have a hidden exit
+ * that the verifier must account for.
+ */
+static int visit_abnormal_return_insn(struct bpf_verifier_env *env, int t)
 {
 	static struct bpf_subprog_info *subprog;
 	struct bpf_iarray *jt;
@@ -19311,6 +19332,13 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 	/* All non-branch instructions have a single fall-through edge. */
 	if (BPF_CLASS(insn->code) != BPF_JMP &&
 	    BPF_CLASS(insn->code) != BPF_JMP32) {
+		if (BPF_CLASS(insn->code) == BPF_LD &&
+		    (BPF_MODE(insn->code) == BPF_ABS ||
+		     BPF_MODE(insn->code) == BPF_IND)) {
+			ret = visit_abnormal_return_insn(env, t);
+			if (ret)
+				return ret;
+		}
 		insn_sz = bpf_is_ldimm64(insn) ? 2 : 1;
 		return push_insn(t, t + insn_sz, FALLTHROUGH, env);
 	}
@@ -19356,7 +19384,7 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
 			if (bpf_helper_changes_pkt_data(insn->imm))
 				mark_subprog_changes_pkt_data(env, t);
 			if (insn->imm == BPF_FUNC_tail_call) {
-				ret = visit_tailcall_insn(env, t);
+				ret = visit_abnormal_return_insn(env, t);
 				if (ret)
 					return ret;
 			}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer
  2026-04-08 15:37 [PATCH bpf-next 1/4] bpf: Propagate error from visit_tailcall_insn Daniel Borkmann
  2026-04-08 15:37 ` [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs Daniel Borkmann
@ 2026-04-08 15:37 ` Daniel Borkmann
  2026-04-08 16:21   ` Anton Protopopov
  2026-04-08 15:37 ` [PATCH bpf-next 4/4] selftests/bpf: Add tests for ld_{abs,ind} failure path in subprogs Daniel Borkmann
  2 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 15:37 UTC (permalink / raw)
  To: bpf; +Cc: ast, eddyz87, info

The local subprog pointer in create_jt() and visit_abnormal_return_insn()
was declared static.

It is unconditionally assigned via bpf_find_containing_subprog() before
every use. Thus, the static qualifier serves no purpose and rather creates
confusion. Just remove it.

Fixes: e40f5a6bf88a ("bpf: correct stack liveness for tail calls")
Fixes: 493d9e0d6083 ("bpf, x86: add support for indirect jumps")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 46f55ea6684e..6cb54764d452 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19224,7 +19224,7 @@ static struct bpf_iarray *jt_from_subprog(struct bpf_verifier_env *env,
 static struct bpf_iarray *
 create_jt(int t, struct bpf_verifier_env *env)
 {
-	static struct bpf_subprog_info *subprog;
+	struct bpf_subprog_info *subprog;
 	int subprog_start, subprog_end;
 	struct bpf_iarray *jt;
 	int i;
@@ -19299,7 +19299,7 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env)
  */
 static int visit_abnormal_return_insn(struct bpf_verifier_env *env, int t)
 {
-	static struct bpf_subprog_info *subprog;
+	struct bpf_subprog_info *subprog;
 	struct bpf_iarray *jt;
 
 	if (env->insn_aux_data[t].jt)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH bpf-next 4/4] selftests/bpf: Add tests for ld_{abs,ind} failure path in subprogs
  2026-04-08 15:37 [PATCH bpf-next 1/4] bpf: Propagate error from visit_tailcall_insn Daniel Borkmann
  2026-04-08 15:37 ` [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs Daniel Borkmann
  2026-04-08 15:37 ` [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer Daniel Borkmann
@ 2026-04-08 15:37 ` Daniel Borkmann
  2 siblings, 0 replies; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 15:37 UTC (permalink / raw)
  To: bpf; +Cc: ast, eddyz87, info

Extend the verifier_ld_ind BPF selftests with subprogs containing
ld_{abs,ind} and craft the test in a way where the invalid register
read is rejected in the fixed case. Also add a success case each,
and add additional coverage related to the BTF return type enforcement.

  # LDLIBS=-static PKG_CONFIG='pkg-config --static' ./vmtest.sh -- ./test_progs -t verifier_ld_ind
  [...]
  #611/1   verifier_ld_ind/ld_ind: check calling conv, r1:OK
  #611/2   verifier_ld_ind/ld_ind: check calling conv, r1 @unpriv:OK
  #611/3   verifier_ld_ind/ld_ind: check calling conv, r2:OK
  #611/4   verifier_ld_ind/ld_ind: check calling conv, r2 @unpriv:OK
  #611/5   verifier_ld_ind/ld_ind: check calling conv, r3:OK
  #611/6   verifier_ld_ind/ld_ind: check calling conv, r3 @unpriv:OK
  #611/7   verifier_ld_ind/ld_ind: check calling conv, r4:OK
  #611/8   verifier_ld_ind/ld_ind: check calling conv, r4 @unpriv:OK
  #611/9   verifier_ld_ind/ld_ind: check calling conv, r5:OK
  #611/10  verifier_ld_ind/ld_ind: check calling conv, r5 @unpriv:OK
  #611/11  verifier_ld_ind/ld_ind: check calling conv, r7:OK
  #611/12  verifier_ld_ind/ld_ind: check calling conv, r7 @unpriv:OK
  #611/13  verifier_ld_ind/ld_abs: subprog early exit on ld_abs failure:OK
  #611/14  verifier_ld_ind/ld_ind: subprog early exit on ld_ind failure:OK
  #611/15  verifier_ld_ind/ld_abs: subprog with both paths safe:OK
  #611/16  verifier_ld_ind/ld_ind: subprog with both paths safe:OK
  #611/17  verifier_ld_ind/ld_abs: reject void return subprog:OK
  #611/18  verifier_ld_ind/ld_ind: reject void return subprog:OK
  #611     verifier_ld_ind:OK
  Summary: 1/18 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 .../selftests/bpf/progs/verifier_ld_ind.c     | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_ld_ind.c b/tools/testing/selftests/bpf/progs/verifier_ld_ind.c
index c925ba9a2e74..09e81b99eecb 100644
--- a/tools/testing/selftests/bpf/progs/verifier_ld_ind.c
+++ b/tools/testing/selftests/bpf/progs/verifier_ld_ind.c
@@ -107,4 +107,146 @@ __naked void ind_check_calling_conv_r7(void)
 	: __clobber_all);
 }
 
+/*
+ * ld_{abs,ind} subprog that always sets r0=1 on the success path.
+ * bpf_gen_ld_abs() emits a hidden exit with r0=0 when the load helper
+ * fails. The verifier must model this failure return so that callers
+ * account for r0=0 as a possible return value.
+ */
+__naked __noinline __used
+static int ldabs_subprog(void)
+{
+	asm volatile (
+	"r6 = r1;"
+	".8byte %[ld_abs];"
+	"r0 = 1;"
+	"exit;"
+	:
+	: __imm_insn(ld_abs, BPF_LD_ABS(BPF_W, 0))
+	: __clobber_all);
+}
+
+__naked __noinline __used
+static int ldind_subprog(void)
+{
+	asm volatile (
+	"r6 = r1;"
+	"r7 = 0;"
+	".8byte %[ld_ind];"
+	"r0 = 1;"
+	"exit;"
+	:
+	: __imm_insn(ld_ind, BPF_LD_IND(BPF_W, BPF_REG_7, 0))
+	: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_abs: subprog early exit on ld_abs failure")
+__failure __msg("R9 !read_ok")
+__naked void ld_abs_subprog_early_exit(void)
+{
+	asm volatile (
+	"call ldabs_subprog;"
+	"if r0 != 0 goto l_exit_%=;"
+	"r0 = r9;"
+	"l_exit_%=:"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_ind: subprog early exit on ld_ind failure")
+__failure __msg("R9 !read_ok")
+__naked void ld_ind_subprog_early_exit(void)
+{
+	asm volatile (
+	"call ldind_subprog;"
+	"if r0 != 0 goto l_exit_%=;"
+	"r0 = r9;"
+	"l_exit_%=:"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_abs: subprog with both paths safe")
+__success
+__naked void ld_abs_subprog_both_paths_safe(void)
+{
+	asm volatile (
+	"call ldabs_subprog;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_ind: subprog with both paths safe")
+__success
+__naked void ld_ind_subprog_both_paths_safe(void)
+{
+	asm volatile (
+	"call ldind_subprog;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+/*
+ * ld_{abs,ind} in subprogs require scalar (int) return type in BTF.
+ * A test with void return must be rejected.
+ */
+__naked __noinline __used
+static void ldabs_void_subprog(void)
+{
+	asm volatile (
+	"r6 = r1;"
+	".8byte %[ld_abs];"
+	"r0 = 1;"
+	"exit;"
+	:
+	: __imm_insn(ld_abs, BPF_LD_ABS(BPF_W, 0))
+	: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_abs: reject void return subprog")
+__failure __msg("LD_ABS is only allowed in functions that return 'int'")
+__naked void ld_abs_void_subprog_reject(void)
+{
+	asm volatile (
+	"call ldabs_void_subprog;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+__naked __noinline __used
+static void ldind_void_subprog(void)
+{
+	asm volatile (
+	"r6 = r1;"
+	"r7 = 0;"
+	".8byte %[ld_ind];"
+	"r0 = 1;"
+	"exit;"
+	:
+	: __imm_insn(ld_ind, BPF_LD_IND(BPF_W, BPF_REG_7, 0))
+	: __clobber_all);
+}
+
+SEC("socket")
+__description("ld_ind: reject void return subprog")
+__failure __msg("LD_ABS is only allowed in functions that return 'int'")
+__naked void ld_ind_void_subprog_reject(void)
+{
+	asm volatile (
+	"call ldind_void_subprog;"
+	"r0 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs
  2026-04-08 15:37 ` [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs Daniel Borkmann
@ 2026-04-08 16:20   ` Alexei Starovoitov
  2026-04-08 17:04     ` Daniel Borkmann
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2026-04-08 16:20 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: bpf, Alexei Starovoitov, Eduard, STAR Labs SG

On Wed, Apr 8, 2026 at 8:37 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> Usage of ld_{abs,ind} instructions got extended into subprogs some time
> ago via commit 09b28d76eac4 ("bpf: Add abnormal return checks."). These
> are only allowed in subprograms when the latter are BTF annotated and
> have scalar return types.
>
> The code generator in bpf_gen_ld_abs() has an abnormal exit path (r0=0 +
> exit) from legacy cBPF times. While the enforcement is on scalar return
> types, the verifier must also simulate the path of abnormal exit if the
> packet data load via ld_{abs,ind} failed.
>
> This is currently not the case. Fix it by having the verifier simulate
> both success and failure paths, and extend it in similar ways as we do
> for tail calls. The success path (r0=unknown, continue to next insn) is
> pushed onto stack for later validation and the r0=0 and return to the
> caller is done on the fall-through side.
>
> Fixes: 09b28d76eac4 ("bpf: Add abnormal return checks.")
> Reported-by: STAR Labs SG <info@starlabs.sg>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index db009d509ade..46f55ea6684e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -18357,6 +18357,22 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
>         mark_reg_unknown(env, regs, BPF_REG_0);
>         /* ld_abs load up to 32-bit skb data. */
>         regs[BPF_REG_0].subreg_def = env->insn_idx + 1;
> +       /*
> +        * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0
> +        * which must be explored by the verifier when in a subprog.
> +        */
> +       if (env->cur_state->curframe) {
> +               struct bpf_verifier_state *branch;
> +
> +               branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
> +               if (IS_ERR(branch))
> +                       return PTR_ERR(branch);
> +               mark_reg_known_zero(env, regs, BPF_REG_0);
> +               err = prepare_func_exit(env, &env->insn_idx);
> +               if (err)
> +                       return err;
> +               env->insn_idx--;

similar code in check_helper_call() for tail_call
also does mark_reg_scratched() and clear_all_pkt_pointers()
which are missing here.

Pls refactor that part into a helper and call it here.

pw-bot: cr

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer
  2026-04-08 15:37 ` [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer Daniel Borkmann
@ 2026-04-08 16:21   ` Anton Protopopov
  0 siblings, 0 replies; 9+ messages in thread
From: Anton Protopopov @ 2026-04-08 16:21 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: bpf, ast, eddyz87, info

On 26/04/08 05:37PM, Daniel Borkmann wrote:
> The local subprog pointer in create_jt() and visit_abnormal_return_insn()
> was declared static.
> 
> It is unconditionally assigned via bpf_find_containing_subprog() before
> every use. Thus, the static qualifier serves no purpose and rather creates
> confusion. Just remove it.
> 
> Fixes: e40f5a6bf88a ("bpf: correct stack liveness for tail calls")
> Fixes: 493d9e0d6083 ("bpf, x86: add support for indirect jumps")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  kernel/bpf/verifier.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 46f55ea6684e..6cb54764d452 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19224,7 +19224,7 @@ static struct bpf_iarray *jt_from_subprog(struct bpf_verifier_env *env,
>  static struct bpf_iarray *
>  create_jt(int t, struct bpf_verifier_env *env)
>  {
> -	static struct bpf_subprog_info *subprog;
> +	struct bpf_subprog_info *subprog;

I wonder where I copy-pasted this from... Thanks!

Acked-by: Anton Protopopov <a.s.protopopov@gmail.com>

>  	int subprog_start, subprog_end;
>  	struct bpf_iarray *jt;
>  	int i;
> @@ -19299,7 +19299,7 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env)
>   */
>  static int visit_abnormal_return_insn(struct bpf_verifier_env *env, int t)
>  {
> -	static struct bpf_subprog_info *subprog;
> +	struct bpf_subprog_info *subprog;
>  	struct bpf_iarray *jt;
>  
>  	if (env->insn_aux_data[t].jt)
> -- 
> 2.43.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs
  2026-04-08 16:20   ` Alexei Starovoitov
@ 2026-04-08 17:04     ` Daniel Borkmann
  2026-04-08 17:47       ` Alexei Starovoitov
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 17:04 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: bpf, Alexei Starovoitov, Eduard, STAR Labs SG

On 4/8/26 6:20 PM, Alexei Starovoitov wrote:
> On Wed, Apr 8, 2026 at 8:37 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> Usage of ld_{abs,ind} instructions got extended into subprogs some time
>> ago via commit 09b28d76eac4 ("bpf: Add abnormal return checks."). These
>> are only allowed in subprograms when the latter are BTF annotated and
>> have scalar return types.
>>
>> The code generator in bpf_gen_ld_abs() has an abnormal exit path (r0=0 +
>> exit) from legacy cBPF times. While the enforcement is on scalar return
>> types, the verifier must also simulate the path of abnormal exit if the
>> packet data load via ld_{abs,ind} failed.
>>
>> This is currently not the case. Fix it by having the verifier simulate
>> both success and failure paths, and extend it in similar ways as we do
>> for tail calls. The success path (r0=unknown, continue to next insn) is
>> pushed onto stack for later validation and the r0=0 and return to the
>> caller is done on the fall-through side.
>>
>> Fixes: 09b28d76eac4 ("bpf: Add abnormal return checks.")
>> Reported-by: STAR Labs SG <info@starlabs.sg>
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>>   kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index db009d509ade..46f55ea6684e 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -18357,6 +18357,22 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
>>          mark_reg_unknown(env, regs, BPF_REG_0);
>>          /* ld_abs load up to 32-bit skb data. */
>>          regs[BPF_REG_0].subreg_def = env->insn_idx + 1;
>> +       /*
>> +        * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0
>> +        * which must be explored by the verifier when in a subprog.
>> +        */
>> +       if (env->cur_state->curframe) {
>> +               struct bpf_verifier_state *branch;
>> +
>> +               branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
>> +               if (IS_ERR(branch))
>> +                       return PTR_ERR(branch);
>> +               mark_reg_known_zero(env, regs, BPF_REG_0);
>> +               err = prepare_func_exit(env, &env->insn_idx);
>> +               if (err)
>> +                       return err;
>> +               env->insn_idx--;
> 
> similar code in check_helper_call() for tail_call
> also does mark_reg_scratched() and clear_all_pkt_pointers()
> which are missing here.

The clear_all_pkt_pointers() is only relevant for the cases where we
modify/pull/push skb data, but in ld_{abs,ind} we either read skb
packet data directly in linear section or via skb_copy_bits.

I'll add the mark_reg_scratched(env, BPF_REG_0). Not sure how much
this really suits to move into a common helper: the tail call case
calls clear_all_pkt_pointers() and marks r0 unknown right after
push_stack(), the ld_{abs,ind} case does not clear the packet
pointers and marks r0 as known 0. imo its fine to leave it separate.

> Pls refactor that part into a helper and call it here.
> 
> pw-bot: cr


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs
  2026-04-08 17:04     ` Daniel Borkmann
@ 2026-04-08 17:47       ` Alexei Starovoitov
  2026-04-08 18:47         ` Daniel Borkmann
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2026-04-08 17:47 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: bpf, Alexei Starovoitov, Eduard, STAR Labs SG

On Wed, Apr 8, 2026 at 10:04 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 4/8/26 6:20 PM, Alexei Starovoitov wrote:
> > On Wed, Apr 8, 2026 at 8:37 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> Usage of ld_{abs,ind} instructions got extended into subprogs some time
> >> ago via commit 09b28d76eac4 ("bpf: Add abnormal return checks."). These
> >> are only allowed in subprograms when the latter are BTF annotated and
> >> have scalar return types.
> >>
> >> The code generator in bpf_gen_ld_abs() has an abnormal exit path (r0=0 +
> >> exit) from legacy cBPF times. While the enforcement is on scalar return
> >> types, the verifier must also simulate the path of abnormal exit if the
> >> packet data load via ld_{abs,ind} failed.
> >>
> >> This is currently not the case. Fix it by having the verifier simulate
> >> both success and failure paths, and extend it in similar ways as we do
> >> for tail calls. The success path (r0=unknown, continue to next insn) is
> >> pushed onto stack for later validation and the r0=0 and return to the
> >> caller is done on the fall-through side.
> >>
> >> Fixes: 09b28d76eac4 ("bpf: Add abnormal return checks.")
> >> Reported-by: STAR Labs SG <info@starlabs.sg>
> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> >> ---
> >>   kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++++++++--
> >>   1 file changed, 30 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index db009d509ade..46f55ea6684e 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -18357,6 +18357,22 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
> >>          mark_reg_unknown(env, regs, BPF_REG_0);
> >>          /* ld_abs load up to 32-bit skb data. */
> >>          regs[BPF_REG_0].subreg_def = env->insn_idx + 1;
> >> +       /*
> >> +        * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0
> >> +        * which must be explored by the verifier when in a subprog.
> >> +        */
> >> +       if (env->cur_state->curframe) {
> >> +               struct bpf_verifier_state *branch;
> >> +
> >> +               branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
> >> +               if (IS_ERR(branch))
> >> +                       return PTR_ERR(branch);
> >> +               mark_reg_known_zero(env, regs, BPF_REG_0);
> >> +               err = prepare_func_exit(env, &env->insn_idx);
> >> +               if (err)
> >> +                       return err;
> >> +               env->insn_idx--;
> >
> > similar code in check_helper_call() for tail_call
> > also does mark_reg_scratched() and clear_all_pkt_pointers()
> > which are missing here.
>
> The clear_all_pkt_pointers() is only relevant for the cases where we
> modify/pull/push skb data, but in ld_{abs,ind} we either read skb
> packet data directly in linear section or via skb_copy_bits.
>
> I'll add the mark_reg_scratched(env, BPF_REG_0). Not sure how much
> this really suits to move into a common helper: the tail call case
> calls clear_all_pkt_pointers() and marks r0 unknown right after
> push_stack(), the ld_{abs,ind} case does not clear the packet
> pointers and marks r0 as known 0. imo its fine to leave it separate.

Ahh. Known zero vs unknown. I missed that. Makes sense.

btw what is the point of env->insn_idx--; ?
This and tail_call case are doing it, but why?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs
  2026-04-08 17:47       ` Alexei Starovoitov
@ 2026-04-08 18:47         ` Daniel Borkmann
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Borkmann @ 2026-04-08 18:47 UTC (permalink / raw)
  To: Alexei Starovoitov; +Cc: bpf, Alexei Starovoitov, Eduard, STAR Labs SG

On 4/8/26 7:47 PM, Alexei Starovoitov wrote:
> On Wed, Apr 8, 2026 at 10:04 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 4/8/26 6:20 PM, Alexei Starovoitov wrote:
>>> On Wed, Apr 8, 2026 at 8:37 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>
>>>> Usage of ld_{abs,ind} instructions got extended into subprogs some time
>>>> ago via commit 09b28d76eac4 ("bpf: Add abnormal return checks."). These
>>>> are only allowed in subprograms when the latter are BTF annotated and
>>>> have scalar return types.
>>>>
>>>> The code generator in bpf_gen_ld_abs() has an abnormal exit path (r0=0 +
>>>> exit) from legacy cBPF times. While the enforcement is on scalar return
>>>> types, the verifier must also simulate the path of abnormal exit if the
>>>> packet data load via ld_{abs,ind} failed.
>>>>
>>>> This is currently not the case. Fix it by having the verifier simulate
>>>> both success and failure paths, and extend it in similar ways as we do
>>>> for tail calls. The success path (r0=unknown, continue to next insn) is
>>>> pushed onto stack for later validation and the r0=0 and return to the
>>>> caller is done on the fall-through side.
>>>>
>>>> Fixes: 09b28d76eac4 ("bpf: Add abnormal return checks.")
>>>> Reported-by: STAR Labs SG <info@starlabs.sg>
>>>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>>>> ---
>>>>    kernel/bpf/verifier.c | 32 ++++++++++++++++++++++++++++++--
>>>>    1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index db009d509ade..46f55ea6684e 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -18357,6 +18357,22 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
>>>>           mark_reg_unknown(env, regs, BPF_REG_0);
>>>>           /* ld_abs load up to 32-bit skb data. */
>>>>           regs[BPF_REG_0].subreg_def = env->insn_idx + 1;
>>>> +       /*
>>>> +        * See bpf_gen_ld_abs() which emits a hidden BPF_EXIT with r0=0
>>>> +        * which must be explored by the verifier when in a subprog.
>>>> +        */
>>>> +       if (env->cur_state->curframe) {
>>>> +               struct bpf_verifier_state *branch;
>>>> +
>>>> +               branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false);
>>>> +               if (IS_ERR(branch))
>>>> +                       return PTR_ERR(branch);
>>>> +               mark_reg_known_zero(env, regs, BPF_REG_0);
>>>> +               err = prepare_func_exit(env, &env->insn_idx);
>>>> +               if (err)
>>>> +                       return err;
>>>> +               env->insn_idx--;
>>>
>>> similar code in check_helper_call() for tail_call
>>> also does mark_reg_scratched() and clear_all_pkt_pointers()
>>> which are missing here.
>>
>> The clear_all_pkt_pointers() is only relevant for the cases where we
>> modify/pull/push skb data, but in ld_{abs,ind} we either read skb
>> packet data directly in linear section or via skb_copy_bits.
>>
>> I'll add the mark_reg_scratched(env, BPF_REG_0). Not sure how much
>> this really suits to move into a common helper: the tail call case
>> calls clear_all_pkt_pointers() and marks r0 unknown right after
>> push_stack(), the ld_{abs,ind} case does not clear the packet
>> pointers and marks r0 as known 0. imo its fine to leave it separate.
> 
> Ahh. Known zero vs unknown. I missed that. Makes sense.
> 
> btw what is the point of env->insn_idx--; ?
> This and tail_call case are doing it, but why?

The prepare_func_exit() sets env->insn_idx to the caller's return
site (aka the insn after the subprog call). do_check_insn() does
env->insn_idx++ at the end of the function to tell "we're done
processing this insn". So we need the env->insn_idx-- otherwise
we're overshooting/skipping validation by one insn.

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-04-08 18:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-08 15:37 [PATCH bpf-next 1/4] bpf: Propagate error from visit_tailcall_insn Daniel Borkmann
2026-04-08 15:37 ` [PATCH bpf-next 2/4] bpf: Fix ld_{abs,ind} failure path analysis in subprogs Daniel Borkmann
2026-04-08 16:20   ` Alexei Starovoitov
2026-04-08 17:04     ` Daniel Borkmann
2026-04-08 17:47       ` Alexei Starovoitov
2026-04-08 18:47         ` Daniel Borkmann
2026-04-08 15:37 ` [PATCH bpf-next 3/4] bpf: Remove static qualifier from local subprog pointer Daniel Borkmann
2026-04-08 16:21   ` Anton Protopopov
2026-04-08 15:37 ` [PATCH bpf-next 4/4] selftests/bpf: Add tests for ld_{abs,ind} failure path in subprogs Daniel Borkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox