Re: [PATCH bpf-next v4 3/3] bpf: Avoid redundant min()/max() in cnum signed bounds

> diff --git a/kernel/bpf/cnum_defs.h b/kernel/bpf/cnum_defs.h
> index 09535edbbafc..30685e43de04 100644
> --- a/kernel/bpf/cnum_defs.h
> +++ b/kernel/bpf/cnum_defs.h

[ ... ]

> @@ -67,19 +80,18 @@ static inline bool FN(srange_overflow)(struct cnum_t cnum)
>   * cnum{T}_smin / cnum{T}_smax query a signed range represented by this cnum.
>   * If cnum represents a range crossing the ST_MAX/ST_MIN boundary, the unbound range
>   * [ST_MIN..ST_MAX] is returned.
> + *
> + * The caller should ensure !is_empty(cnum) holds when calling
> + * cnum{T}_smin / cnum{T}_smax.
>   */
>  st FN(smin)(struct cnum_t cnum)
>  {
> -	return FN(srange_overflow)(cnum)
> -	       ? ST_MIN
> -	       : min((st)cnum.base, (st)(cnum.base + cnum.size));
> +	return FN(srange_overflow)(cnum) ? ST_MIN : (st)cnum.base;
>  }
>
>  st FN(smax)(struct cnum_t cnum)
>  {
> -	return FN(srange_overflow)(cnum)
> -	       ? ST_MAX
> -	       : max((st)cnum.base, (st)(cnum.base + cnum.size));
> +	return FN(srange_overflow)(cnum) ? ST_MAX : (st)(cnum.base + cnum.size);
>  }

Does the new precondition match how these functions are actually called?

The patch adds comments stating callers should ensure !is_empty(cnum)
holds, but the in-tree callers in include/linux/bpf_verifier.h
reg_smin() / reg_smax() / reg_s32_min() / reg_s32_max() forward reg->r64
and reg->r32 directly into these functions with no is_empty() test.

Those accessors are used at around 69 sites in kernel/bpf/verifier.c,
including bounds arithmetic that feeds memory-access checking (like
min_off/max_off and scalar_min_max_mul).

The change alters observable behaviour for the EMPTY sentinel
{base=UT_MAX, size=UT_MAX}. For this value srange_overflow() returns
false because FN(contains) short-circuits false on empty cnums, so the
non-overflow branch executes.

With the old code:

    smin = min((st)UT_MAX, (st)(UT_MAX + UT_MAX)) = min(-1, -2) = -2
    smax = max((st)UT_MAX, (st)(UT_MAX + UT_MAX)) = max(-1, -2) = -1

which gives smin <= smax (ordered).

With the new code:

    smin = (st)UT_MAX                          = -1
    smax = (st)(UT_MAX + UT_MAX)               = -2

which gives smin > smax (inverted).

So where the old implementation accidentally returned a well-ordered
pair for the one input the new comments admit is out of contract, the
new implementation returns an inverted pair. This narrows the safety
margin: the defensive ordering that held for every possible input no
longer holds when an unchecked EMPTY reaches these functions.

range_bounds_violation() gates every path that commits a register into
regs[], and reg_bounds_sync() early-returns on an empty range, which is
conservative. I could not construct a soundness escape. But the
precondition is documented without being enforced or asserted, and the
callers are not audited for that contract.

Should the precondition be enforced at the call sites, or should
smin()/smax() keep returning an ordered pair for EMPTY?


---
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/30534991827