Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value

Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value

[Zac Ecob]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

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.

[-- Attachment #2: repro.tar.xz --]
[-- Type: application/x-xz, Size: 2540 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Fixed-sign-extension-issue-in-coerce_subreg_to_size_.patch --]
[-- Type: text/x-patch; name=0001-Fixed-sign-extension-issue-in-coerce_subreg_to_size_.patch, Size: 1212 bytes --]

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
+		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

Re: Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value

[Stanislav Fomichev]

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
>