diff mbox series

[v16,02/12] bpf: Move dynptr type check to is_dynptr_type_expected()

Message ID 20220905143318.1592015-3-roberto.sassu@huaweicloud.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series bpf: Add kfuncs for PKCS#7 signature verification | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
netdev/tree_selection success Guessing tree name failed - patch did not apply, async
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on s390x with gcc

Commit Message

Roberto Sassu Sept. 5, 2022, 2:33 p.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

Move dynptr type check to is_dynptr_type_expected() from
is_dynptr_reg_valid_init(), so that callers can better determine the cause
of a negative result (dynamic pointer not valid/initialized, dynamic
pointer of the wrong type). It will be useful for example for BTF, to
restrict which dynamic pointer types can be passed to kfuncs, as initially
only the local type will be supported.

Also, splitting makes the code more readable, since checking the dynamic
pointer type is not necessarily related to validity and initialization.

Split the validity/initialization and dynamic pointer type check also in
the verifier, and adjust the expected error message in the test (a test for
an unexpected dynptr type passed to a helper cannot be added due to missing
suitable helpers, but this case has been tested manually).

Cc: Joanne Koong <joannelkoong@gmail.com>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 kernel/bpf/verifier.c                         | 35 ++++++++++++++-----
 .../testing/selftests/bpf/prog_tests/dynptr.c |  2 +-
 2 files changed, 28 insertions(+), 9 deletions(-)

Comments

Kumar Kartikeya Dwivedi Sept. 6, 2022, 2:32 a.m. UTC | #1 
On Mon, 5 Sept 2022 at 16:34, Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> Move dynptr type check to is_dynptr_type_expected() from
> is_dynptr_reg_valid_init(), so that callers can better determine the cause
> of a negative result (dynamic pointer not valid/initialized, dynamic
> pointer of the wrong type). It will be useful for example for BTF, to
> restrict which dynamic pointer types can be passed to kfuncs, as initially
> only the local type will be supported.
>
> Also, splitting makes the code more readable, since checking the dynamic
> pointer type is not necessarily related to validity and initialization.
>
> Split the validity/initialization and dynamic pointer type check also in
> the verifier, and adjust the expected error message in the test (a test for
> an unexpected dynptr type passed to a helper cannot be added due to missing
> suitable helpers, but this case has been tested manually).
>
> Cc: Joanne Koong <joannelkoong@gmail.com>
> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

I'm not sure the split is _really_ needed, but I don't feel strongly
about it and defer to Joanne and others.
Otherwise,
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

>  kernel/bpf/verifier.c                         | 35 ++++++++++++++-----
>  .../testing/selftests/bpf/prog_tests/dynptr.c |  2 +-
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 068b20ed34d2..10b3c0a81d09 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -779,8 +779,8 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
>         return true;
>  }
>
> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
> -                                    enum bpf_arg_type arg_type)
> +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
> +                                    struct bpf_reg_state *reg)
>  {
>         struct bpf_func_state *state = func(env, reg);
>         int spi = get_spi(reg->off);
> @@ -796,11 +796,24 @@ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
>                         return false;
>         }
>
> +       return true;
> +}
> +
> +static bool is_dynptr_type_expected(struct bpf_verifier_env *env,
> +                                   struct bpf_reg_state *reg,
> +                                   enum bpf_arg_type arg_type)
> +{
> +       struct bpf_func_state *state = func(env, reg);
> +       enum bpf_dynptr_type dynptr_type;
> +       int spi = get_spi(reg->off);
> +
>         /* ARG_PTR_TO_DYNPTR takes any type of dynptr */
>         if (arg_type == ARG_PTR_TO_DYNPTR)
>                 return true;
>
> -       return state->stack[spi].spilled_ptr.dynptr.type == arg_to_dynptr_type(arg_type);
> +       dynptr_type = arg_to_dynptr_type(arg_type);
> +
> +       return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
>  }
>
>  /* The reg state of a pointer or a bounded scalar was saved when
> @@ -6050,21 +6063,27 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>                         }
>
>                         meta->uninit_dynptr_regno = regno;
> -               } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
> +               } else if (!is_dynptr_reg_valid_init(env, reg)) {
> +                       verbose(env,
> +                               "Expected an initialized dynptr as arg #%d\n",
> +                               arg + 1);
> +                       return -EINVAL;
> +               } else if (!is_dynptr_type_expected(env, reg, arg_type)) {
>                         const char *err_extra = "";
>
>                         switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
>                         case DYNPTR_TYPE_LOCAL:
> -                               err_extra = "local ";
> +                               err_extra = "local";
>                                 break;
>                         case DYNPTR_TYPE_RINGBUF:
> -                               err_extra = "ringbuf ";
> +                               err_extra = "ringbuf";
>                                 break;
>                         default:
> +                               err_extra = "<unknown>";
>                                 break;
>                         }
> -
> -                       verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
> +                       verbose(env,
> +                               "Expected a dynptr of type %s as arg #%d\n",
>                                 err_extra, arg + 1);
>                         return -EINVAL;
>                 }
> diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> index bcf80b9f7c27..8fc4e6c02bfd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
> +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
> @@ -30,7 +30,7 @@ static struct {
>         {"invalid_helper2", "Expected an initialized dynptr as arg #3"},
>         {"invalid_write1", "Expected an initialized dynptr as arg #1"},
>         {"invalid_write2", "Expected an initialized dynptr as arg #3"},
> -       {"invalid_write3", "Expected an initialized ringbuf dynptr as arg #1"},
> +       {"invalid_write3", "Expected an initialized dynptr as arg #1"},
>         {"invalid_write4", "arg 1 is an unacquired reference"},
>         {"invalid_read1", "invalid read from stack"},
>         {"invalid_read2", "cannot pass in dynptr at an offset"},
> --
> 2.25.1
>
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 068b20ed34d2..10b3c0a81d09 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -779,8 +779,8 @@  static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
 	return true;
 }
 
-static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg,
-				     enum bpf_arg_type arg_type)
+static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env,
+				     struct bpf_reg_state *reg)
 {
 	struct bpf_func_state *state = func(env, reg);
 	int spi = get_spi(reg->off);
@@ -796,11 +796,24 @@  static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_re
 			return false;
 	}
 
+	return true;
+}
+
+static bool is_dynptr_type_expected(struct bpf_verifier_env *env,
+				    struct bpf_reg_state *reg,
+				    enum bpf_arg_type arg_type)
+{
+	struct bpf_func_state *state = func(env, reg);
+	enum bpf_dynptr_type dynptr_type;
+	int spi = get_spi(reg->off);
+
 	/* ARG_PTR_TO_DYNPTR takes any type of dynptr */
 	if (arg_type == ARG_PTR_TO_DYNPTR)
 		return true;
 
-	return state->stack[spi].spilled_ptr.dynptr.type == arg_to_dynptr_type(arg_type);
+	dynptr_type = arg_to_dynptr_type(arg_type);
+
+	return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
 }
 
 /* The reg state of a pointer or a bounded scalar was saved when
@@ -6050,21 +6063,27 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 			}
 
 			meta->uninit_dynptr_regno = regno;
-		} else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) {
+		} else if (!is_dynptr_reg_valid_init(env, reg)) {
+			verbose(env,
+				"Expected an initialized dynptr as arg #%d\n",
+				arg + 1);
+			return -EINVAL;
+		} else if (!is_dynptr_type_expected(env, reg, arg_type)) {
 			const char *err_extra = "";
 
 			switch (arg_type & DYNPTR_TYPE_FLAG_MASK) {
 			case DYNPTR_TYPE_LOCAL:
-				err_extra = "local ";
+				err_extra = "local";
 				break;
 			case DYNPTR_TYPE_RINGBUF:
-				err_extra = "ringbuf ";
+				err_extra = "ringbuf";
 				break;
 			default:
+				err_extra = "<unknown>";
 				break;
 			}
-
-			verbose(env, "Expected an initialized %sdynptr as arg #%d\n",
+			verbose(env,
+				"Expected a dynptr of type %s as arg #%d\n",
 				err_extra, arg + 1);
 			return -EINVAL;
 		}
diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c
index bcf80b9f7c27..8fc4e6c02bfd 100644
--- a/tools/testing/selftests/bpf/prog_tests/dynptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c
@@ -30,7 +30,7 @@  static struct {
 	{"invalid_helper2", "Expected an initialized dynptr as arg #3"},
 	{"invalid_write1", "Expected an initialized dynptr as arg #1"},
 	{"invalid_write2", "Expected an initialized dynptr as arg #3"},
-	{"invalid_write3", "Expected an initialized ringbuf dynptr as arg #1"},
+	{"invalid_write3", "Expected an initialized dynptr as arg #1"},
 	{"invalid_write4", "arg 1 is an unacquired reference"},
 	{"invalid_read1", "invalid read from stack"},
 	{"invalid_read2", "cannot pass in dynptr at an offset"},