Re: [RFC bpf-next 2/6] bpf: move the linked-scalar flags into bpf_reg_state->flags [NFC]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing BPF_FLAG_ADD_CONST check in the explore_alu_limits verification path allows incorrect pruning of incompatible linked scalars.
- [Medium] Narrowing spills of scalar registers fail to clear BPF_FLAG_ADD_CONST linkage flags, leading to stale flags on unlinked registers and subsequent pruning failures in regsafe().
--

--- Patch [2]: [RFC bpf-next 2/6] bpf: move the linked-scalar flags into bpf_reg_state->flags [NFC] ---
commit 19636caada7b86a42b8056f18241af3d2e662845
Author: Vineet Gupta <[email protected]>
Subject: bpf: move the linked-scalar flags into bpf_reg_state->flags [NFC]

This patch moves the linked-scalar flags from the bpf_reg_state->id field
into a dedicated bpf_reg_state->flags field. It also updates the verification
logic to explicitly check these new flags using the link_flags_match helper
where appropriate.

> diff --git a/kernel/bpf/states.c b/kernel/bpf/states.c
> index f7a0314fa1064..d3105b9a9965f 100644
> --- a/kernel/bpf/states.c
> +++ b/kernel/bpf/states.c
> @@ -488,11 +480,32 @@ static int clean_verifier_state(struct bpf_verifier_env *env,

[ ... ]

> +static bool link_flags_match(const struct bpf_reg_state *rold,
> +			     const struct bpf_reg_state *rcur)
> +{
> +	if (!rold->id)
> +		return true;
> +
> +	return (rold->flags & BPF_FLAG_ADD_CONST) == (rcur->flags & BPF_FLAG_ADD_CONST);
> +}
> +
>  static bool regs_exact(const struct bpf_reg_state *rold,
>  		       const struct bpf_reg_state *rcur,
>  		       struct bpf_idmap *idmap)
>  {
>  	return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
> +	       link_flags_match(rold, rcur) &&
>  	       check_ids(rold->id, rcur->id, idmap) &&
>  	       check_ids(rold->parent_id, rcur->parent_id, idmap);
>  }

[Severity: High]
Does the explore_alu_limits verification path also need a similar update?

In regsafe(), when env->explore_alu_limits is true, the verifier uses a
strict comparison:

regsafe() {
        if (env->explore_alu_limits) {
                return memcmp(rold, rcur, offsetof(struct bpf_reg_state, id)) == 0 &&
                       check_scalar_ids(rold->id, rcur->id, idmap);
        }
}

Since memcmp() stops at the id field, it misses the newly added flags
field. Without calling link_flags_match(), states with the same base
scalar IDs but different linkage flags (such as BPF_FLAG_ADD_CONST32 vs
BPF_FLAG_ADD_CONST64) might be incorrectly deemed equivalent, bypassing
the strict bounds checking intended by this mode.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8925749d636e6..93e69116ca9ed 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -3308,6 +3309,7 @@ static void clear_scalar_id(struct bpf_reg_state *reg)
>  {
>  	reg->id = 0;
>  	reg->delta = 0;
> +	reg->flags &= ~BPF_FLAG_ADD_CONST;
>  }

[Severity: Medium]
Can narrowing spills leave stale flags on unlinked registers?

When a linked scalar register is spilled with a smaller size,
check_stack_write_fixed_off() copies the register and manually breaks
the relation by setting the id to 0:

check_stack_write_fixed_off() {
        save_register_state(env, state, spi, reg, size);
        if (!reg_value_fits)
                state->stack[spi].spilled_ptr.id = 0;
}

Since the linkage flags were moved out of the id field, setting id to 0
no longer clears the BPF_FLAG_ADD_CONST bits.

If this state is later compared in regsafe(), link_flags_match() will see
rold->id != 0, but rcur->id == 0 with a stale BPF_FLAG_ADD_CONST flag.
Because the rold->id check does not short-circuit for rcur, the helper
will evaluate to false, incorrectly rejecting a state that is safe to
prune.

Would it be safer to use clear_scalar_id() or otherwise clear the flags
when breaking the relation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2