Re: [PATCH bpf-next v4 04/46] bpf: Add join of registers and stack slots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] bpf: join_reg() zeroes dst->delta under explore_alu_limits,
  failing regsafe()
- [Medium] bpf: loss of STACK_POISON in join_slot() when joining stack
  slots
--

commit b2b35d77649236d79410bf6f82762993660918fc
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Add join of registers and stack slots

This commit introduces state joining for registers and stack slots, along
with SCEV tracking of linear registers for loop-head widening.

> diff --git a/kernel/bpf/join.c b/kernel/bpf/join.c
> new file mode 100644
> index 0000000000000..fe513b7a8e884
> --- /dev/null
> +++ b/kernel/bpf/join.c

[ ... ]

> +static bool join_reg(struct bpf_join_state *js, struct bpf_reg_state *dst,
> +		     const struct bpf_reg_state *ra, const struct bpf_reg_state *rb, int pass)
> +{
> +	if (ra->type == NOT_INIT || rb->type == NOT_INIT) {
> +		if (pass == 2 && ra->type != NOT_INIT)
> +			*dst = *rb;
> +		return true;
> +	}
> +
> +	if (ra->type != rb->type)
> +		return false;
> +
> +	switch (base_type(ra->type)) {
> +	case SCALAR_VALUE:
> +		if (js->env->explore_alu_limits && !reg_value_equal(ra, rb))
> +			return false;
> +		if (pass == 1) {
> +			jid_note(js, ra->id, rb->id, scalar_link_consistent(ra, rb));
> +			return true;
> +		}
> +		join_range(js, dst, ra, rb);
> +		/* a link that exists on one side only is dropped */
> +		if (ra->id && !jid_good(js, ra->id, rb->id)) {
> +			dst->id = 0;
> +			dst->delta = 0;
> +		}
> +		return true;

[Severity: Medium]
As reported by bot+bpf-ci@kernel.org on v2 [1], when js->env->explore_alu_limits
is set, can zeroing dst->delta here make the joined register fail regsafe()
against ra and rb?

When ra and rb have matching non-zero deltas (BPF_ADD_CONST) and identical
ranges, reg_value_equal(ra, rb) succeeds, but if !jid_good(js, ra->id, rb->id)
drops the link in pass 2 (for instance, when one path sets BPF_ADD_CONST64
and the other sets BPF_ADD_CONST32, or on an ID pairing conflict), dst->delta
is cleared to 0.  Because s32 delta sits before id in struct bpf_reg_state,
regsafe() under explore_alu_limits compares delta via memcmp(...,
offsetof(struct bpf_reg_state, id)), causing bpf_state_subsumes() to fail,
triggering verifier_bug() and rejecting the program with -EFAULT.

[ ... ]

> +static bool join_slot(struct bpf_join_state *js, struct bpf_stack_state *ds,
> +		      const struct bpf_stack_state *sa, const struct bpf_stack_state *sb, int pass)
> +{

[ ... ]

> +	for (i = 0; i < BPF_REG_SIZE; i++) {
> +		u8 x = sa->slot_type[i], y = sb->slot_type[i];
> +
> +		if (x == STACK_SPILL)
> +			x = STACK_MISC;
> +		if (y == STACK_SPILL)
> +			y = STACK_MISC;
> +		if (stype_is_invalid(x) && stype_is_invalid(y))
> +			res[i] = sa->slot_type[i];
> +		else if (stype_is_invalid(x) || stype_is_invalid(y))
> +			res[i] = STACK_INVALID;

[Severity: Medium]
Does this overwrite STACK_POISON with STACK_INVALID when joining a poisoned
stack byte with STACK_INVALID, STACK_MISC, STACK_ZERO, or a scalar
STACK_SPILL?

For example, when only the low 4-byte half of a slot is live,
__clean_func_state() leaves the dead upper half of a 64-bit constant-zero
scalar spill as STACK_SPILL in one state while marking it STACK_POISON in
another.  Degrading STACK_SPILL to STACK_MISC and joining it with
STACK_POISON produces STACK_INVALID, which allows subsequent reads of dead
poisoned stack slots to succeed under allow_uninit_stack instead of being
rejected.  Checking whether x == STACK_POISON or y == STACK_POISON first and
setting res[i] = STACK_POISON would preserve poison tracking while remaining
compatible with stacksafe().

[1] https://lore.kernel.org/r/ee77c73bff0b56a40098080a5335d5fd5e4d8592ac15afe769c1c4661f026576@mail.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107033758.1547725-1-alexei.starovoitov@gmail.com?part=4