Re: [PATCH bpf-next v1 15/29] bpf: Describe handles in registers at throw sites

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: failed state joins on dead registers in
  bpf_states_join()
--

commit 76f36da94410f1998945cf52f9a2b12ce1afb269
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Describe handles in registers at throw sites

This commit records resource handles held in registers in site descriptors
and recovers callee-saved registers across frames during exception unwinding
so that references held in registers at throw sites do not require hidden
stack slots.

> diff --git a/kernel/bpf/join.c b/kernel/bpf/join.c
> index 4ec834e31dff5..180b0a96029cf 100644
> --- a/kernel/bpf/join.c
> +++ b/kernel/bpf/join.c
> @@ -663,6 +663,9 @@ int bpf_states_join(struct bpf_verifier_env *env, struct bpf_verifier_state *dst
>  			u16 live_regs = env->insn_aux_data[insn_idx].live_regs_before;
>  			u32 cnt;
>  
> +			/* the registers both keep for a throw are joined, see bpf_frame_desc_keeps() */
> +			live_regs |= bpf_frame_desc_kept_regs(env, a, fr) &
> +				     bpf_frame_desc_kept_regs(env, b, fr);

[Severity: Medium]
Can adding bpf_frame_desc_kept_regs() to live_regs cause bpf_states_join()
to fail when a dead register holds resource pointers with different offsets
or IDs in states a and b?

bpf_frame_desc_kept_regs() sets BIT(reg) whenever reg points to any held
resource at any constant s16 offset.  If a dead register holds p + 0 on one
iteration of a loop and p + 1 on the next, BIT(reg) is set in both masks, so
join_func() runs join_reg() on the dead register.  join_reg() rejects the
mismatched pointer offsets and aborts the entire state join instead of
clearing the dead register to NOT_INIT in dst, preventing loop widening and
causing verification to fail with -E2BIG once flow_merge() reaches
BPF_FLOW_MAX_STATES.  Similarly, if the dead register holds two different
held resources in a and b, join_reg() in pass 1 calls jid_note() on their
distinct IDs, poisoning js->pairs and failing join_refs_check() in pass 2.

In addition, flow_unset_differs() assumes that only live_regs_before
registers remain initialized after bpf_clean_verifier_state():

kernel/bpf/flow.c:flow_unset_differs() {
	for (fr = 0; fr <= a->curframe; fr++)
		for (i = BPF_REG_0; i < BPF_REG_FP; i++)
			if ((a->frame[fr]->regs[i].type == NOT_INIT) !=
			    (b->frame[fr]->regs[i].type == NOT_INIT))
				return true;
	return false;
}

When a dead register is kept by bpf_frame_desc_kept_regs() in only one of
the two states, flow_unset_differs() returns true and prevents flow_merge()
and flow_end_walk() from joining them while flow_has_room() is true.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794399387.git.memxor@gmail.com?part=15