Re: [PATCH bpf-next v2 07/13] bpf: track low-32 scalar equality across sign-extending movs

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4189 bytes --]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index eb093194e2a3..308ff53232f0 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -16248,12 +16258,23 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
>  						return -EACCES;
>  					} else if (src_reg->type == SCALAR_VALUE) {
>  						bool no_sext;
> +						/*
> +						 * A 32-bit sign extension keeps the low 32
> +						 * bits, so record a low-32 link as the
> +						 * zero-extending mov does. A self-mov
> +						 * qualifies only if src is already linked.
> +						 */
> +						bool subreg_link = (insn->off >> 3) == 4 &&
> +								   (src_reg != dst_reg ||
> +								    src_reg->id);
>
>  						no_sext = reg_umax(src_reg) < (1ULL << (insn->off - 1));
> -						if (no_sext)
> +						if (no_sext || subreg_link)
>  							assign_scalar_id_before_mov(env, src_reg);
                                                        ^^^^

Can widening this guard drop an existing ADD_CONST link on the source
register?  With 'no_sext || subreg_link', assign_scalar_id_before_mov() now
also runs on src_reg when !no_sext, and that helper discards the source's
delta tracking:

kernel/bpf/verifier.c:assign_scalar_id_before_mov() {
	...
	if (src_reg->add_const)
		clear_scalar_id(src_reg);
	if (!src_reg->id && !tnum_is_const(src_reg->var_off))
		src_reg->id = ++env->id_gen;
	...
}

clear_scalar_id() zeroes id, delta, add_const and subreg, and the following
statement then mints a brand-new id, so src_reg is unlinked from the
register set it was tracking.  Before this commit the !no_sext arm left
src_reg untouched.

For a BPF_ALU64 | BPF_MOV | BPF_X with insn->off == 32:

	r1 = r2			/* r1, r2 share id N */
	r1 += 8			/* r1->add_const = ADD_CONST_64, r1->delta = 8 */
	r3 = (s32)r1		/* r1 is wide -> !no_sext, subreg_link true */
	if r2 < 100 goto out	/* sync_linked_regs() no longer reaches r1 */
	... use r1 ...

At the mov, r1 loses its ADD_CONST_64 / delta = 8 link and gets a fresh id,
so the following 'if r2 < 100' compare no longer refines r1 to [8, 107].
Is that the intent?  This is the compiler idiom described in the comment
above adjust_reg_min_max_vals()'s delta handling.

The clear also does not look necessary for the new link: since !no_sext, the
SUBREG_SEXT link is established through the freshly minted id.

Would excluding add_const sources be enough, something like:

	bool subreg_link = (insn->off >> 3) == 4 && !src_reg->add_const &&
			   (src_reg != dst_reg || src_reg->id);

The zero-extending arm added earlier in the series uses the same guard
shape, so a fix likely wants to cover both.  Looking forward through
2c5433cf9fbb..1d3c4a0831be, the later commits extend the same mechanism to
narrowing stack fills and spills and reuse the identical guard, so none of
them change this.

>  						*dst_reg = *src_reg;
> -						if (!no_sext)
> +						if (!no_sext && subreg_link && src_reg->id)
> +							dst_reg->subreg = SUBREG_SEXT;
                                                        ^^^^

On a related note, for 'r0 = (s32)r0' where r0 carries add_const,
subreg_link is evaluated before the clear and sees the old src_reg->id as
non-zero, so it is true.  assign_scalar_id_before_mov() then clears and
re-mints the id, and SUBREG_SEXT is applied to a register that is the sole
holder of that id.

The changelog says:

> Unlike the zero-extending arm, a self-mov can form a link here, but only
> when src is already linked: r0 = (s32)r0 is how a sign-extended int return
> lands. On an unlinked register there is nothing to link to, and minting an
> id would leave the register describing itself.

bpf_clear_singular_ids() drops singleton ids before state comparison, so
this does not look unsound, but does it match the rationale above?

> +						else if (!no_sext)
>  							clear_scalar_id(dst_reg);
>  						coerce_reg_to_size_sx(dst_reg, insn->off >> 3);
>  					} else {

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34506184282