diff mbox series

Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value

Message ID h3qKLDEO6m9nhif0eAQX4fVrqdO0D_OPb0y5HfMK9jBePEKK33wQ3K-bqSVnr0hiZdFZtSJOsbNkcEQGpv_yJk61PAAiO8fUkgMRSO-lB50=@protonmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Fixing coerce_subreg_to_size_sx invalidly setting reg->umax_value | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc

Commit Message

Zac Ecob July 11, 2024, 8:07 a.m. UTC 
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.

Comments

Stanislav Fomichev July 12, 2024, 1:16 a.m. UTC | #1 
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
>
diff mbox series

Patch

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