Re: Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value

[Stanislav Fomichev <sdf@fomichev.me>]

On 07/11, Zac Ecob wrote:
> Hi,
> 
> My fuzzer recently found another bug, in which `reg->umax_value` is being invalidly set in regards to sign extensions.
> 
> The lines below contain the bug:
> ```
> reg->umin_value = reg->u32_min_value = s64_min;                                                             
> reg->umax_value = reg->u32_max_value = s64_max;
> ```
> 
> If `s64_min` / `s64_max` are negative values here, they correctly cast when assigning to the u32 values. However, when assigned to `umin_value` / `umax_value`, it seems there is an implicit (u32) cast applied, causing the top 32 bits to not be set.
> 
> 
> I've attached the files to reproduce, as well as the patch file, based off of 6.10-rc4 - albeit this is my first patch so I'd appreciate someone checking it's formatted fine.
> 
> Thanks.


> From da5ef523f7cd018f3f0991454a18bc961ea1abba Mon Sep 17 00:00:00 2001
> From: Zac Ecob <zacecob@protonmail.com>
> Date: Thu, 11 Jul 2024 17:41:55 +1000
> Subject: [PATCH] Fixed sign-extension issue in coerce_subreg_to_size_sx
> 
> ---
>  kernel/bpf/verifier.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 010a6eb864dc..eccf3ac8996a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6213,8 +6213,14 @@ static void coerce_reg_to_size_sx(struct bpf_reg_state *reg, int size)
>  	if ((s64_max >= 0) == (s64_min >= 0)) {
>  		reg->smin_value = reg->s32_min_value = s64_min;
>  		reg->smax_value = reg->s32_max_value = s64_max;
> -		reg->umin_value = reg->u32_min_value = s64_min;
> -		reg->umax_value = reg->u32_max_value = s64_max;
> +
> +		// Cannot chain assignments, like reg->umax_val = reg->u32_max_val = (signed input)
> +		// Because of the implicit cast leading to reg->umax_val not being properly set for negative numbers

Pls use /* */ comments instead, use [PATCH bpf] subject in a followup and
try to find a commit that introduced the problem to mention it in the
'Fixes:' tag.

Also, instead of your custom reproducer, can you add a small reproducer
to the test_verifier.c (tools/testing/selftests/bpf/verifier/**) to
demonstrate the issue and avoid similar regressions in the future?

> +		reg->u32_min_value = s64_min;
> +		reg->u32_max_value = s64_max;
> +		reg->umin_value    = s64_min;
> +		reg->umax_value    = s64_max;
> +
>  		reg->var_off = tnum_range(s64_min, s64_max);
>  		return;
>  	}
> -- 
> 2.30.2
>