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@xxxxxxxxxxxxxx>
> 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
>
