diff mbox series

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

Message ID 20220830161716.754078-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
netdev/tree_selection success Guessing tree name failed - patch did not apply, async
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-4 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
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-7 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps 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-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-13 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_verifier on s390x 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

Commit Message

Roberto Sassu Aug. 30, 2022, 4:17 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).

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

Joanne Koong Aug. 30, 2022, 4:46 p.m. UTC | #1 
On Tue, Aug 30, 2022 at 9:18 AM 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).
>
> Also, splitting makes the code more readable, since checking the dynamic
> pointer type is not necessarily related to validity and initialization.

I think it'd be helpful to also include that btf will be using these
functions, which seems like the main motivation behind why this change
is needed.

>
> 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).

The bpf_ringbuf_submit_dynptr() and bpf_ringbuf_discard_dynptr()
helpers take in only ringbuf-type dynptrs, so either of these would
work for testing the case where an incorrect dynptr type is passed in
:)

>
> 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(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0194a36d0b36..1b913db252a3 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);
> +       int spi = get_spi(reg->off);
> +       enum bpf_dynptr_type dynptr_type;

nit: the above 2 lines should be swapped to maintain reverse christmas
tree order of declarations

> +
>         /* 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
>
Roberto Sassu Aug. 31, 2022, 9:22 a.m. UTC | #2 
On Tue, 2022-08-30 at 09:46 -0700, Joanne Koong wrote:
> On Tue, Aug 30, 2022 at 9:18 AM 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).
> > 
> > Also, splitting makes the code more readable, since checking the
> > dynamic
> > pointer type is not necessarily related to validity and
> > initialization.
> 
> I think it'd be helpful to also include that btf will be using these
> functions, which seems like the main motivation behind why this
> change
> is needed.

Ok, added.

> > 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).
> 
> The bpf_ringbuf_submit_dynptr() and bpf_ringbuf_discard_dynptr()
> helpers take in only ringbuf-type dynptrs, so either of these would
> work for testing the case where an incorrect dynptr type is passed in
> :)

Uhm, that didn't work.

If I initialize a local dynptr, and call bpf_ringbuf_submit_dynptr(), I
get:

arg 1 is an unacquired reference

The only way to make this work was to create a custom helper, with
OBJ_RELEASE removed.

> > 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(-)
> > 
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 0194a36d0b36..1b913db252a3 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);
> > +       int spi = get_spi(reg->off);
> > +       enum bpf_dynptr_type dynptr_type;
> 
> nit: the above 2 lines should be swapped to maintain reverse
> christmas
> tree order of declarations

Ok.

Thanks

Roberto

> 
> > +
> >         /* 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 0194a36d0b36..1b913db252a3 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);
+	int spi = get_spi(reg->off);
+	enum bpf_dynptr_type dynptr_type;
+
 	/* 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"},