diff mbox series

bpf: Refactor ptr alu checking rules to allow alu explicitly

Message ID 20240117094012.36798-1-sunhao.th@gmail.com (mailing list archive)
State Accepted
Commit 2ce793ebe207328b1210bb53effd702740987148
Delegated to: BPF
Headers show
Series bpf: Refactor ptr alu checking rules to allow alu explicitly | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply
bpf/vmtest-bpf-PR fail PR summary
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-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
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-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
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-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-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 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-34 success Logs for x86_64-llvm-17 / veristat
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-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-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-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-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-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-39 fail 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-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
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-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-14 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-15 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-16 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-28 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-25 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-29 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-32 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-36 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-37 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-9 pending Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-6 pending Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-4 pending Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 pending Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-13 pending Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-14 pending Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-11 pending Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release

Commit Message

Hao Sun Jan. 17, 2024, 9:40 a.m. UTC 
Current checking rules are structured to disallow alu on particular ptr
types explicitly, so default cases are allowed implicitly. This may lead
to newly added ptr types being allowed unexpectedly. So restruture it to
allow alu explicitly. The tradeoff is mainly a bit more cases added in
the switch. The following table from Eduard summarizes the rules:

        | Pointer type        | Arithmetics allowed |
        |---------------------+---------------------|
        | PTR_TO_CTX          | yes                 |
        | CONST_PTR_TO_MAP    | conditionally       |
        | PTR_TO_MAP_VALUE    | yes                 |
        | PTR_TO_MAP_KEY      | yes                 |
        | PTR_TO_STACK        | yes                 |
        | PTR_TO_PACKET_META  | yes                 |
        | PTR_TO_PACKET       | yes                 |
        | PTR_TO_PACKET_END   | no                  |
        | PTR_TO_FLOW_KEYS    | conditionally       |
        | PTR_TO_SOCKET       | no                  |
        | PTR_TO_SOCK_COMMON  | no                  |
        | PTR_TO_TCP_SOCK     | no                  |
        | PTR_TO_TP_BUFFER    | yes                 |
        | PTR_TO_XDP_SOCK     | no                  |
        | PTR_TO_BTF_ID       | yes                 |
        | PTR_TO_MEM          | yes                 |
        | PTR_TO_BUF          | yes                 |
        | PTR_TO_FUNC         | yes                 |
        | CONST_PTR_TO_DYNPTR | yes                 |

The refactored rules are equivalent to the original one. Note that
PTR_TO_FUNC and CONST_PTR_TO_DYNPTR are not reject here because: (1)
check_mem_access() rejects load/store on those ptrs, and those ptrs
with offset passing to calls are rejected check_func_arg_reg_off();
(2) someone may rely on the verifier not rejecting programs earily.

Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
 kernel/bpf/verifier.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Hao Sun Jan. 17, 2024, 9:42 a.m. UTC | #1 
On Wed, Jan 17, 2024 at 10:40 AM Hao Sun <sunhao.th@gmail.com> wrote:
>
> Current checking rules are structured to disallow alu on particular ptr
> types explicitly, so default cases are allowed implicitly. This may lead
> to newly added ptr types being allowed unexpectedly. So restruture it to
> allow alu explicitly. The tradeoff is mainly a bit more cases added in
> the switch. The following table from Eduard summarizes the rules:
>
>         | Pointer type        | Arithmetics allowed |
>         |---------------------+---------------------|
>         | PTR_TO_CTX          | yes                 |
>         | CONST_PTR_TO_MAP    | conditionally       |
>         | PTR_TO_MAP_VALUE    | yes                 |
>         | PTR_TO_MAP_KEY      | yes                 |
>         | PTR_TO_STACK        | yes                 |
>         | PTR_TO_PACKET_META  | yes                 |
>         | PTR_TO_PACKET       | yes                 |
>         | PTR_TO_PACKET_END   | no                  |
>         | PTR_TO_FLOW_KEYS    | conditionally       |
>         | PTR_TO_SOCKET       | no                  |
>         | PTR_TO_SOCK_COMMON  | no                  |
>         | PTR_TO_TCP_SOCK     | no                  |
>         | PTR_TO_TP_BUFFER    | yes                 |
>         | PTR_TO_XDP_SOCK     | no                  |
>         | PTR_TO_BTF_ID       | yes                 |
>         | PTR_TO_MEM          | yes                 |
>         | PTR_TO_BUF          | yes                 |
>         | PTR_TO_FUNC         | yes                 |
>         | CONST_PTR_TO_DYNPTR | yes                 |
>
> The refactored rules are equivalent to the original one. Note that
> PTR_TO_FUNC and CONST_PTR_TO_DYNPTR are not reject here because: (1)
> check_mem_access() rejects load/store on those ptrs, and those ptrs
> with offset passing to calls are rejected check_func_arg_reg_off();
> (2) someone may rely on the verifier not rejecting programs earily.
>
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> ---

Not specifying bpf-next as the target repo as my previous patch is not
in it yet.
Eduard Zingerman Jan. 18, 2024, 2:38 p.m. UTC | #2 
On Wed, 2024-01-17 at 10:40 +0100, Hao Sun wrote:
> Current checking rules are structured to disallow alu on particular ptr
> types explicitly, so default cases are allowed implicitly. This may lead
> to newly added ptr types being allowed unexpectedly. So restruture it to
> allow alu explicitly. The tradeoff is mainly a bit more cases added in
> the switch. The following table from Eduard summarizes the rules:
> 
>         | Pointer type        | Arithmetics allowed |
>         |---------------------+---------------------|
>         | PTR_TO_CTX          | yes                 |
>         | CONST_PTR_TO_MAP    | conditionally       |
>         | PTR_TO_MAP_VALUE    | yes                 |
>         | PTR_TO_MAP_KEY      | yes                 |
>         | PTR_TO_STACK        | yes                 |
>         | PTR_TO_PACKET_META  | yes                 |
>         | PTR_TO_PACKET       | yes                 |
>         | PTR_TO_PACKET_END   | no                  |
>         | PTR_TO_FLOW_KEYS    | conditionally       |
>         | PTR_TO_SOCKET       | no                  |
>         | PTR_TO_SOCK_COMMON  | no                  |
>         | PTR_TO_TCP_SOCK     | no                  |
>         | PTR_TO_TP_BUFFER    | yes                 |
>         | PTR_TO_XDP_SOCK     | no                  |
>         | PTR_TO_BTF_ID       | yes                 |
>         | PTR_TO_MEM          | yes                 |
>         | PTR_TO_BUF          | yes                 |
>         | PTR_TO_FUNC         | yes                 |
>         | CONST_PTR_TO_DYNPTR | yes                 |
> 
> The refactored rules are equivalent to the original one. Note that
> PTR_TO_FUNC and CONST_PTR_TO_DYNPTR are not reject here because: (1)
> check_mem_access() rejects load/store on those ptrs, and those ptrs
> with offset passing to calls are rejected check_func_arg_reg_off();
> (2) someone may rely on the verifier not rejecting programs earily.
> 
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> ---

Tried this on top of "Reject variable offset alu on PTR_TO_FLOW_KEYS",
all seems to be ok.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
patchwork-bot+netdevbpf@kernel.org Jan. 23, 2024, 11:40 p.m. UTC | #3 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 17 Jan 2024 10:40:12 +0100 you wrote:
> Current checking rules are structured to disallow alu on particular ptr
> types explicitly, so default cases are allowed implicitly. This may lead
> to newly added ptr types being allowed unexpectedly. So restruture it to
> allow alu explicitly. The tradeoff is mainly a bit more cases added in
> the switch. The following table from Eduard summarizes the rules:
> 
>         | Pointer type        | Arithmetics allowed |
>         |---------------------+---------------------|
>         | PTR_TO_CTX          | yes                 |
>         | CONST_PTR_TO_MAP    | conditionally       |
>         | PTR_TO_MAP_VALUE    | yes                 |
>         | PTR_TO_MAP_KEY      | yes                 |
>         | PTR_TO_STACK        | yes                 |
>         | PTR_TO_PACKET_META  | yes                 |
>         | PTR_TO_PACKET       | yes                 |
>         | PTR_TO_PACKET_END   | no                  |
>         | PTR_TO_FLOW_KEYS    | conditionally       |
>         | PTR_TO_SOCKET       | no                  |
>         | PTR_TO_SOCK_COMMON  | no                  |
>         | PTR_TO_TCP_SOCK     | no                  |
>         | PTR_TO_TP_BUFFER    | yes                 |
>         | PTR_TO_XDP_SOCK     | no                  |
>         | PTR_TO_BTF_ID       | yes                 |
>         | PTR_TO_MEM          | yes                 |
>         | PTR_TO_BUF          | yes                 |
>         | PTR_TO_FUNC         | yes                 |
>         | CONST_PTR_TO_DYNPTR | yes                 |
> 
> [...]

Here is the summary with links:
  - bpf: Refactor ptr alu checking rules to allow alu explicitly
    https://git.kernel.org/bpf/bpf-next/c/2ce793ebe207

You are awesome, thank you!
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 65f598694d55..861d8d824bb8 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12826,6 +12826,19 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	}
 
 	switch (base_type(ptr_reg->type)) {
+	case PTR_TO_CTX:
+	case PTR_TO_MAP_VALUE:
+	case PTR_TO_MAP_KEY:
+	case PTR_TO_STACK:
+	case PTR_TO_PACKET_META:
+	case PTR_TO_PACKET:
+	case PTR_TO_TP_BUFFER:
+	case PTR_TO_BTF_ID:
+	case PTR_TO_MEM:
+	case PTR_TO_BUF:
+	case PTR_TO_FUNC:
+	case CONST_PTR_TO_DYNPTR:
+		break;
 	case PTR_TO_FLOW_KEYS:
 		if (known)
 			break;
@@ -12835,16 +12848,10 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		if (known && smin_val == 0 && opcode == BPF_ADD)
 			break;
 		fallthrough;
-	case PTR_TO_PACKET_END:
-	case PTR_TO_SOCKET:
-	case PTR_TO_SOCK_COMMON:
-	case PTR_TO_TCP_SOCK:
-	case PTR_TO_XDP_SOCK:
+	default:
 		verbose(env, "R%d pointer arithmetic on %s prohibited\n",
 			dst, reg_type_str(env, ptr_reg->type));
 		return -EACCES;
-	default:
-		break;
 	}
 
 	/* In case of 'scalar += pointer', dst_reg inherits pointer type and id.