Re: [RFC PATCH 6.1.y 1/2] bpf: drop knowledge-losing __reg_combine_{32,64}_into_{64,32} logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The removal of `__reg_combine_32_into_64()` and `__reg_combine_64_into_32()` drops critical precision-preserving fast paths, causing the verifier to artificially widen bounds and incorrectly reject valid BPF programs.

In upstream Linux, this patch was part of a major 'BPF register bounds logic rewrite' series, which completely overhauled `reg_bounds_sync()` to algebraically deduce 64-bit bounds from 32-bit bounds (and vice versa) without losing precision. However, in this isolated backport to 6.1.y, the rewritten `reg_bounds_sync()` infrastructure was not included. The 6.1.y `reg_bounds_sync()` relies entirely on `var_off` (tnum) approximations.

Before this patch, `__reg_combine_32_into_64()` explicitly bypassed this tnum approximation when the upper 32 bits of a register were known to be zero, directly copying tight 32-bit bounds into the 64-bit bounds using `__reg_assign_32_into_64()`. Similarly, `__reg_combine_64_into_32()` copied tight 64-bit bounds back to 32-bit bounds.

By dropping these helpers and falling back to the 6.1.y `reg_bounds_sync()`, a tight 32-bit bound like `[0, 5]` is converted to a tnum mask `7`, which then widens the 64-bit `umax_value` to `7`. Because the verifier relies on tight 64-bit bounds to enforce safe memory accesses, widening them artificially will cause the verifier to incorrectly reject safe programs.
--

commit 276e00bfbaf18530ce686f7167a23c7b6257d77a
Author: Zhenzhong Wu <jt26wzz@gmail.com>

bpf: drop knowledge-losing __reg_combine_{32,64}_into_{64,32} logic

This commit removes __reg_combine_32_into_64() and __reg_combine_64_into_32()
and replaces them with reg_bounds_sync() calls. This was part of a larger
upstream series rewriting register bounds logic.

Link: https://lore.kernel.org/r/20231102033759.2541186-10-andrii@kernel.org

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 264b3dc714cc4..d9b027f3dd121 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -4443,9 +4398,10 @@ static void coerce_reg_to_size(struct bpf_reg_state *reg, int size)
>  	 * values are also truncated so we push 64-bit bounds into
>  	 * 32-bit bounds. Above were truncated < 32-bits already.
>  	 */
> -	if (size >= 4)
> -		return;
> -	__reg_combine_64_into_32(reg);
> +	if (size < 4) {
> +		__mark_reg32_unbounded(reg);
> +		reg_bounds_sync(reg);
> +	}
>  }

[Severity: High]
Does this replacement in coerce_reg_to_size() lose bounds precision on older
kernels like 6.1.y?

Before this patch, __reg_combine_64_into_32() explicitly copied tight 64-bit
bounds directly into the 32-bit bounds.

It looks like the 6.1.y version of reg_bounds_sync() relies entirely on
var_off (tnum) approximations, because it doesn't include the algebraic
deduction infrastructure that was added in the upstream rewrite series.

Could this mean tight bounds like [0, 5] might be converted to a tnum mask
of 7, artificially widening the bounds and potentially causing the verifier
to incorrectly reject safe programs?

[ ... ]
> @@ -9885,13 +9841,13 @@ static void reg_set_min_max(struct bpf_reg_state *true_reg,
>  					     tnum_subreg(false_32off));
>  		true_reg->var_off = tnum_or(tnum_clear_subreg(true_64off),
>  					    tnum_subreg(true_32off));
> -		__reg_combine_32_into_64(false_reg);
> -		__reg_combine_32_into_64(true_reg);
> +		reg_bounds_sync(false_reg);
> +		reg_bounds_sync(true_reg);
>  	} else {
>  		false_reg->var_off = false_64off;
>  		true_reg->var_off = true_64off;
> -		__reg_combine_64_into_32(false_reg);
> -		__reg_combine_64_into_32(true_reg);
> +		reg_bounds_sync(false_reg);
> +		reg_bounds_sync(true_reg);
>  	}
>  }

[Severity: High]
Will replacing __reg_combine_32_into_64() with reg_bounds_sync() here in
reg_set_min_max() also suffer from the same precision loss?

Previously, when the upper 32 bits were known to be zero,
__reg_combine_32_into_64() used __reg_assign_32_into_64() to directly copy
tight 32-bit bounds to 64-bit bounds, bypassing the tnum approximation.

Without the upstream reg_bounds_sync() algebraic deduction logic, is it
possible this causes bounds widening that breaks existing userspace BPF
applications?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601180400.1381736-1-jt26wzz@gmail.com?part=1