[v5,1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs

  Kfuncs currently support specifying the KF_TRUSTED_ARGS flag to signal
to the verifier that it should enforce that a BPF program passes it a
"safe", trusted pointer. Currently, "safe" means that the pointer is
either PTR_TO_CTX, or is refcounted. There may be cases, however, where
the kernel passes a BPF program a safe / trusted pointer to an object
that the BPF program wishes to use as a kptr, but because the object
does not yet have a ref_obj_id from the perspective of the verifier, the
program would be unable to pass it to a KF_ACQUIRE | KF_TRUSTED_ARGS
kfunc.

The solution is to expand the set of pointers that are considered
trusted according to KF_TRUSTED_ARGS, so that programs can invoke kfuncs
with these pointers without getting rejected by the verifier.

There is already a PTR_UNTRUSTED flag that is set in some scenarios,
such as when a BPF program reads a kptr directly from a map
without performing a bpf_kptr_xchg() call. These pointers of course can
and should be rejected by the verifier. Unfortunately, however,
PTR_UNTRUSTED does not cover all the cases for safety that need to
be addressed to adequately protect kfuncs. Specifically, pointers
obtained by a BPF program "walking" a struct are _not_ considered
PTR_UNTRUSTED according to BPF. For example, say that we were to add a
kfunc called bpf_task_acquire(), with KF_ACQUIRE | KF_TRUSTED_ARGS, to
acquire a struct task_struct *. If we only used PTR_UNTRUSTED to signal
that a task was unsafe to pass to a kfunc, the verifier would mistakenly
allow the following unsafe BPF program to be loaded:

SEC("tp_btf/task_newtask")
int BPF_PROG(unsafe_acquire_task,
             struct task_struct *task,
             u64 clone_flags)
{
        struct task_struct *acquired, *nested;

        nested = task->last_wakee;

        /* Would not be rejected by the verifier. */
        acquired = bpf_task_acquire(nested);
        if (!acquired)
                return 0;

        bpf_task_release(acquired);
        return 0;
}

To address this, this patch defines a new type flag called PTR_NESTED
which tracks whether a PTR_TO_BTF_ID pointer was retrieved from walking
a struct. A pointer passed directly from the kernel begins with
(PTR_NESTED & type) == 0, meaning of course that it is not nested. Any
pointer received from walking that object, however, would inherit that
flag and become a nested pointer.

With that flag, this patch also updates btf_check_func_arg_match() to
only flag a PTR_TO_BTF_ID object as requiring a refcount if it has any
type modifiers (which of course includes both PTR_UNTRUSTED and
PTR_NESTED). Otherwise, the pointer passes this check and continues
onto the others in btf_check_func_arg_match().

A subsequent patch will add kfuncs for storing a task kfunc as a kptr,
and then another patch will validate this feature by ensuring that the
verifier rejects a kfunc invocation with a nested pointer.

Signed-off-by: David Vernet <void@manifault.com>
---
 include/linux/bpf.h                          |  6 ++++++
 kernel/bpf/btf.c                             | 11 ++++++++++-
 kernel/bpf/verifier.c                        | 12 +++++++++++-
 tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
 4 files changed, 29 insertions(+), 4 deletions(-)
  

On Sat, 15 Oct 2022 at 02:51, David Vernet <void@manifault.com> wrote:
>
> Kfuncs currently support specifying the KF_TRUSTED_ARGS flag to signal
> to the verifier that it should enforce that a BPF program passes it a
> "safe", trusted pointer. Currently, "safe" means that the pointer is
> either PTR_TO_CTX, or is refcounted. There may be cases, however, where
> the kernel passes a BPF program a safe / trusted pointer to an object
> that the BPF program wishes to use as a kptr, but because the object
> does not yet have a ref_obj_id from the perspective of the verifier, the
> program would be unable to pass it to a KF_ACQUIRE | KF_TRUSTED_ARGS
> kfunc.
>
> The solution is to expand the set of pointers that are considered
> trusted according to KF_TRUSTED_ARGS, so that programs can invoke kfuncs
> with these pointers without getting rejected by the verifier.
>
> There is already a PTR_UNTRUSTED flag that is set in some scenarios,
> such as when a BPF program reads a kptr directly from a map
> without performing a bpf_kptr_xchg() call. These pointers of course can
> and should be rejected by the verifier. Unfortunately, however,
> PTR_UNTRUSTED does not cover all the cases for safety that need to
> be addressed to adequately protect kfuncs. Specifically, pointers
> obtained by a BPF program "walking" a struct are _not_ considered
> PTR_UNTRUSTED according to BPF. For example, say that we were to add a
> kfunc called bpf_task_acquire(), with KF_ACQUIRE | KF_TRUSTED_ARGS, to
> acquire a struct task_struct *. If we only used PTR_UNTRUSTED to signal
> that a task was unsafe to pass to a kfunc, the verifier would mistakenly
> allow the following unsafe BPF program to be loaded:
>
> SEC("tp_btf/task_newtask")
> int BPF_PROG(unsafe_acquire_task,
>              struct task_struct *task,
>              u64 clone_flags)
> {
>         struct task_struct *acquired, *nested;
>
>         nested = task->last_wakee;
>
>         /* Would not be rejected by the verifier. */
>         acquired = bpf_task_acquire(nested);
>         if (!acquired)
>                 return 0;
>
>         bpf_task_release(acquired);
>         return 0;
> }
>
> To address this, this patch defines a new type flag called PTR_NESTED
> which tracks whether a PTR_TO_BTF_ID pointer was retrieved from walking
> a struct. A pointer passed directly from the kernel begins with
> (PTR_NESTED & type) == 0, meaning of course that it is not nested. Any
> pointer received from walking that object, however, would inherit that
> flag and become a nested pointer.
>
> With that flag, this patch also updates btf_check_func_arg_match() to
> only flag a PTR_TO_BTF_ID object as requiring a refcount if it has any
> type modifiers (which of course includes both PTR_UNTRUSTED and
> PTR_NESTED). Otherwise, the pointer passes this check and continues
> onto the others in btf_check_func_arg_match().
>
> A subsequent patch will add kfuncs for storing a task kfunc as a kptr,
> and then another patch will validate this feature by ensuring that the
> verifier rejects a kfunc invocation with a nested pointer.
>
> Signed-off-by: David Vernet <void@manifault.com>
> ---

Please tag the patches with [ PATCH bpf-next ... ] subject prefix.

>  include/linux/bpf.h                          |  6 ++++++
>  kernel/bpf/btf.c                             | 11 ++++++++++-
>  kernel/bpf/verifier.c                        | 12 +++++++++++-
>  tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
>  4 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9e7d46d16032..b624024edb4e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -457,6 +457,12 @@ enum bpf_type_flag {
>         /* Size is known at compile time. */
>         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
>
> +       /* PTR was obtained from walking a struct. This is used with
> +        * PTR_TO_BTF_ID to determine whether the pointer is safe to pass to a
> +        * kfunc with KF_TRUSTED_ARGS.
> +        */
> +       PTR_NESTED              = BIT(11 + BPF_BASE_TYPE_BITS),
> +
>         __BPF_TYPE_FLAG_MAX,
>         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
>  };
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index eba603cec2c5..3d7bad11b10b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6333,8 +6333,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 /* Check if argument must be a referenced pointer, args + i has
>                  * been verified to be a pointer (after skipping modifiers).
>                  * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> +                *
> +                * All object pointers must be refcounted, other than:
> +                * - PTR_TO_CTX
> +                * - Trusted pointers (i.e. pointers with no type modifiers)
>                  */
> -               if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> +               if (is_kfunc &&
> +                   trusted_args &&
> +                   obj_ptr &&
> +                   base_type(reg->type) != PTR_TO_CTX &&
> +                   type_flag(reg->type) &&
> +                   !reg->ref_obj_id) {
>                         bpf_log(log, "R%d must be referenced\n", regno);
>                         return -EINVAL;
>                 }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6f6d2d511c06..d16a08ca507b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -581,6 +581,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
>                 strncpy(prefix, "user_", 32);
>         if (type & MEM_PERCPU)
>                 strncpy(prefix, "percpu_", 32);
> +       if (type & PTR_NESTED)
> +               strncpy(prefix, "nested_", 32);
>         if (type & PTR_UNTRUSTED)
>                 strncpy(prefix, "untrusted_", 32);
>

Since these are no longer exclusive, the code needs to be updated to
append strings to the prefix buffer.
Maybe just using snprintf with %s%s%s.. would be better, passing ""
when !(type & flag).

> @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>         if (type_flag(reg->type) & PTR_UNTRUSTED)
>                 flag |= PTR_UNTRUSTED;
>
> +       /* All pointers obtained by walking a struct are nested. */
> +       flag |= PTR_NESTED;
> +

Instead of PTR_NESTED, how about PTR_WALK?

>         if (atype == BPF_READ && value_regno >= 0)
>                 mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
>
> @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
>  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
>  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
>  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> +static const struct bpf_reg_types btf_ptr_types = {
> +       .types = {
> +               PTR_TO_BTF_ID,
> +               PTR_TO_BTF_ID | PTR_NESTED
> +       },
> +};
>  static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } };
>  static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_BTF_ID | MEM_PERCPU } };
>  static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
> diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
> index e1a937277b54..496c29b1a298 100644
> --- a/tools/testing/selftests/bpf/verifier/calls.c
> +++ b/tools/testing/selftests/bpf/verifier/calls.c
> @@ -181,7 +181,7 @@
>         },
>         .result_unpriv = REJECT,
>         .result = REJECT,
> -       .errstr = "negative offset ptr_ ptr R1 off=-4 disallowed",
> +       .errstr = "negative offset nested_ptr_ ptr R1 off=-4 disallowed",
>  },
>  {
>         "calls: invalid kfunc call: PTR_TO_BTF_ID with variable offset",
> @@ -243,7 +243,7 @@
>         },
>         .result_unpriv = REJECT,
>         .result = REJECT,
> -       .errstr = "R1 must be referenced",
> +       .errstr = "arg#0 pointer type STRUCT prog_test_ref_kfunc must point to scalar",
>  },
>  {
>         "calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
> --
> 2.38.0
>

On Sat, 15 Oct 2022 at 02:51, David Vernet <void@manifault.com> wrote:
>
> Kfuncs currently support specifying the KF_TRUSTED_ARGS flag to signal
> to the verifier that it should enforce that a BPF program passes it a
> "safe", trusted pointer. Currently, "safe" means that the pointer is
> either PTR_TO_CTX, or is refcounted. There may be cases, however, where
> the kernel passes a BPF program a safe / trusted pointer to an object
> that the BPF program wishes to use as a kptr, but because the object
> does not yet have a ref_obj_id from the perspective of the verifier, the
> program would be unable to pass it to a KF_ACQUIRE | KF_TRUSTED_ARGS
> kfunc.
>
> The solution is to expand the set of pointers that are considered
> trusted according to KF_TRUSTED_ARGS, so that programs can invoke kfuncs
> with these pointers without getting rejected by the verifier.
>
> There is already a PTR_UNTRUSTED flag that is set in some scenarios,
> such as when a BPF program reads a kptr directly from a map
> without performing a bpf_kptr_xchg() call. These pointers of course can
> and should be rejected by the verifier. Unfortunately, however,
> PTR_UNTRUSTED does not cover all the cases for safety that need to
> be addressed to adequately protect kfuncs. Specifically, pointers
> obtained by a BPF program "walking" a struct are _not_ considered
> PTR_UNTRUSTED according to BPF. For example, say that we were to add a
> kfunc called bpf_task_acquire(), with KF_ACQUIRE | KF_TRUSTED_ARGS, to
> acquire a struct task_struct *. If we only used PTR_UNTRUSTED to signal
> that a task was unsafe to pass to a kfunc, the verifier would mistakenly
> allow the following unsafe BPF program to be loaded:
>
> SEC("tp_btf/task_newtask")
> int BPF_PROG(unsafe_acquire_task,
>              struct task_struct *task,
>              u64 clone_flags)
> {
>         struct task_struct *acquired, *nested;
>
>         nested = task->last_wakee;
>
>         /* Would not be rejected by the verifier. */
>         acquired = bpf_task_acquire(nested);
>         if (!acquired)
>                 return 0;
>
>         bpf_task_release(acquired);
>         return 0;
> }
>
> To address this, this patch defines a new type flag called PTR_NESTED
> which tracks whether a PTR_TO_BTF_ID pointer was retrieved from walking
> a struct. A pointer passed directly from the kernel begins with
> (PTR_NESTED & type) == 0, meaning of course that it is not nested. Any
> pointer received from walking that object, however, would inherit that
> flag and become a nested pointer.
>
> With that flag, this patch also updates btf_check_func_arg_match() to
> only flag a PTR_TO_BTF_ID object as requiring a refcount if it has any
> type modifiers (which of course includes both PTR_UNTRUSTED and
> PTR_NESTED). Otherwise, the pointer passes this check and continues
> onto the others in btf_check_func_arg_match().
>
> A subsequent patch will add kfuncs for storing a task kfunc as a kptr,
> and then another patch will validate this feature by ensuring that the
> verifier rejects a kfunc invocation with a nested pointer.
>
> Signed-off-by: David Vernet <void@manifault.com>
> ---
>  include/linux/bpf.h                          |  6 ++++++
>  kernel/bpf/btf.c                             | 11 ++++++++++-
>  kernel/bpf/verifier.c                        | 12 +++++++++++-
>  tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
>  4 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 9e7d46d16032..b624024edb4e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -457,6 +457,12 @@ enum bpf_type_flag {
>         /* Size is known at compile time. */
>         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
>
> +       /* PTR was obtained from walking a struct. This is used with
> +        * PTR_TO_BTF_ID to determine whether the pointer is safe to pass to a
> +        * kfunc with KF_TRUSTED_ARGS.
> +        */
> +       PTR_NESTED              = BIT(11 + BPF_BASE_TYPE_BITS),
> +
>         __BPF_TYPE_FLAG_MAX,
>         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
>  };
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index eba603cec2c5..3d7bad11b10b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6333,8 +6333,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
>                 /* Check if argument must be a referenced pointer, args + i has
>                  * been verified to be a pointer (after skipping modifiers).
>                  * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> +                *
> +                * All object pointers must be refcounted, other than:
> +                * - PTR_TO_CTX
> +                * - Trusted pointers (i.e. pointers with no type modifiers)
>                  */
> -               if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> +               if (is_kfunc &&
> +                   trusted_args &&
> +                   obj_ptr &&
> +                   base_type(reg->type) != PTR_TO_CTX &&
> +                   type_flag(reg->type) &&
> +                   !reg->ref_obj_id) {
>                         bpf_log(log, "R%d must be referenced\n", regno);
>                         return -EINVAL;
>                 }
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6f6d2d511c06..d16a08ca507b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -581,6 +581,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
>                 strncpy(prefix, "user_", 32);
>         if (type & MEM_PERCPU)
>                 strncpy(prefix, "percpu_", 32);
> +       if (type & PTR_NESTED)
> +               strncpy(prefix, "nested_", 32);
>         if (type & PTR_UNTRUSTED)
>                 strncpy(prefix, "untrusted_", 32);
>
> @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>         if (type_flag(reg->type) & PTR_UNTRUSTED)
>                 flag |= PTR_UNTRUSTED;
>
> +       /* All pointers obtained by walking a struct are nested. */
> +       flag |= PTR_NESTED;
> +
>         if (atype == BPF_READ && value_regno >= 0)
>                 mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
>
> @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
>  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
>  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
>  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> +static const struct bpf_reg_types btf_ptr_types = {
> +       .types = {
> +               PTR_TO_BTF_ID,
> +               PTR_TO_BTF_ID | PTR_NESTED
> +       },
> +};

CI fails, two of those failures are from not updating
check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one
is a crash because you didn't add to the list in convert_ctx_access
for PROBE_MEM based load fault handling.

The third issue is a bit more interesting:

; if (fib6_nh->fib_nh_gw_family) {
4375 58: (71) r1 = *(u8 *)(r9 +14)
4376 same insn cannot be used with different pointers

This is because one branch does:
fib6_nh = &rt->fib6_nh[0];
while the other does
fib6_nh = &nh->nh_info->fib6_nh;

So the load insn 58 in one path uses PTR_TO_BTF_ID and in the other
PTR_TO_BTF_ID | PTR_NESTED, so reg_type_mismatch bails on seeing src
!= prev.

I think it's ok to allow this, and we can probably do base_type(src)
!= base_type(prev) here. If the load was not permitted,
check_mem_access before this should have caught it. The check is
mostly to prevent doing stuff like PTR_TO_CTX in one path and
PTR_TO_BTF_ID in the other, because then the convert_ctx_access
handling would be totally incorrect for the load instruction.

Hopefully I'm not missing anything.

But please add some comments about this when updating this particular
check, it's very easy to forget all this later.

> [...]
  

On Tue, Oct 18, 2022 at 07:02:15AM +0530, Kumar Kartikeya Dwivedi wrote:
> Please tag the patches with [ PATCH bpf-next ... ] subject prefix.

Sure, will do.

> >  include/linux/bpf.h                          |  6 ++++++
> >  kernel/bpf/btf.c                             | 11 ++++++++++-
> >  kernel/bpf/verifier.c                        | 12 +++++++++++-
> >  tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
> >  4 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 9e7d46d16032..b624024edb4e 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -457,6 +457,12 @@ enum bpf_type_flag {
> >         /* Size is known at compile time. */
> >         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > +       /* PTR was obtained from walking a struct. This is used with
> > +        * PTR_TO_BTF_ID to determine whether the pointer is safe to pass to a
> > +        * kfunc with KF_TRUSTED_ARGS.
> > +        */
> > +       PTR_NESTED              = BIT(11 + BPF_BASE_TYPE_BITS),
> > +
> >         __BPF_TYPE_FLAG_MAX,
> >         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
> >  };
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index eba603cec2c5..3d7bad11b10b 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6333,8 +6333,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                 /* Check if argument must be a referenced pointer, args + i has
> >                  * been verified to be a pointer (after skipping modifiers).
> >                  * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> > +                *
> > +                * All object pointers must be refcounted, other than:
> > +                * - PTR_TO_CTX
> > +                * - Trusted pointers (i.e. pointers with no type modifiers)
> >                  */
> > -               if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> > +               if (is_kfunc &&
> > +                   trusted_args &&
> > +                   obj_ptr &&
> > +                   base_type(reg->type) != PTR_TO_CTX &&
> > +                   type_flag(reg->type) &&
> > +                   !reg->ref_obj_id) {
> >                         bpf_log(log, "R%d must be referenced\n", regno);
> >                         return -EINVAL;
> >                 }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..d16a08ca507b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -581,6 +581,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> >                 strncpy(prefix, "user_", 32);
> >         if (type & MEM_PERCPU)
> >                 strncpy(prefix, "percpu_", 32);
> > +       if (type & PTR_NESTED)
> > +               strncpy(prefix, "nested_", 32);
> >         if (type & PTR_UNTRUSTED)
> >                 strncpy(prefix, "untrusted_", 32);
> >
> 
> Since these are no longer exclusive, the code needs to be updated to
> append strings to the prefix buffer.
> Maybe just using snprintf with %s%s%s.. would be better, passing ""
> when !(type & flag).

Sure, I can make that change. We'll have to increase the size of the
prefix string on the stack, but that's hardly problematic as these
strings are not terribly large.

> > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> >         if (type_flag(reg->type) & PTR_UNTRUSTED)
> >                 flag |= PTR_UNTRUSTED;
> >
> > +       /* All pointers obtained by walking a struct are nested. */
> > +       flag |= PTR_NESTED;
> > +
> 
> Instead of PTR_NESTED, how about PTR_WALK?

I don't have a strong preference between either, though I would prefer
PTR_WALKED if we go with the latter. Does that work for you?

> >         if (atype == BPF_READ && value_regno >= 0)
> >                 mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
> >
> > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> >  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> >  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> >  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > +static const struct bpf_reg_types btf_ptr_types = {
> > +       .types = {
> > +               PTR_TO_BTF_ID,
> > +               PTR_TO_BTF_ID | PTR_NESTED
> > +       },
> > +};
> >  static const struct bpf_reg_types spin_lock_types = { .types = { PTR_TO_MAP_VALUE } };
> >  static const struct bpf_reg_types percpu_btf_ptr_types = { .types = { PTR_TO_BTF_ID | MEM_PERCPU } };
> >  static const struct bpf_reg_types func_ptr_types = { .types = { PTR_TO_FUNC } };
> > diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
> > index e1a937277b54..496c29b1a298 100644
> > --- a/tools/testing/selftests/bpf/verifier/calls.c
> > +++ b/tools/testing/selftests/bpf/verifier/calls.c
> > @@ -181,7 +181,7 @@
> >         },
> >         .result_unpriv = REJECT,
> >         .result = REJECT,
> > -       .errstr = "negative offset ptr_ ptr R1 off=-4 disallowed",
> > +       .errstr = "negative offset nested_ptr_ ptr R1 off=-4 disallowed",
> >  },
> >  {
> >         "calls: invalid kfunc call: PTR_TO_BTF_ID with variable offset",
> > @@ -243,7 +243,7 @@
> >         },
> >         .result_unpriv = REJECT,
> >         .result = REJECT,
> > -       .errstr = "R1 must be referenced",
> > +       .errstr = "arg#0 pointer type STRUCT prog_test_ref_kfunc must point to scalar",
> >  },
> >  {
> >         "calls: valid kfunc call: referenced arg needs refcounted PTR_TO_BTF_ID",
> > --
> > 2.38.0
> >
> 
> On Sat, 15 Oct 2022 at 02:51, David Vernet <void@manifault.com> wrote:
> >
> > Kfuncs currently support specifying the KF_TRUSTED_ARGS flag to signal
> > to the verifier that it should enforce that a BPF program passes it a
> > "safe", trusted pointer. Currently, "safe" means that the pointer is
> > either PTR_TO_CTX, or is refcounted. There may be cases, however, where
> > the kernel passes a BPF program a safe / trusted pointer to an object
> > that the BPF program wishes to use as a kptr, but because the object
> > does not yet have a ref_obj_id from the perspective of the verifier, the
> > program would be unable to pass it to a KF_ACQUIRE | KF_TRUSTED_ARGS
> > kfunc.
> >
> > The solution is to expand the set of pointers that are considered
> > trusted according to KF_TRUSTED_ARGS, so that programs can invoke kfuncs
> > with these pointers without getting rejected by the verifier.
> >
> > There is already a PTR_UNTRUSTED flag that is set in some scenarios,
> > such as when a BPF program reads a kptr directly from a map
> > without performing a bpf_kptr_xchg() call. These pointers of course can
> > and should be rejected by the verifier. Unfortunately, however,
> > PTR_UNTRUSTED does not cover all the cases for safety that need to
> > be addressed to adequately protect kfuncs. Specifically, pointers
> > obtained by a BPF program "walking" a struct are _not_ considered
> > PTR_UNTRUSTED according to BPF. For example, say that we were to add a
> > kfunc called bpf_task_acquire(), with KF_ACQUIRE | KF_TRUSTED_ARGS, to
> > acquire a struct task_struct *. If we only used PTR_UNTRUSTED to signal
> > that a task was unsafe to pass to a kfunc, the verifier would mistakenly
> > allow the following unsafe BPF program to be loaded:
> >
> > SEC("tp_btf/task_newtask")
> > int BPF_PROG(unsafe_acquire_task,
> >              struct task_struct *task,
> >              u64 clone_flags)
> > {
> >         struct task_struct *acquired, *nested;
> >
> >         nested = task->last_wakee;
> >
> >         /* Would not be rejected by the verifier. */
> >         acquired = bpf_task_acquire(nested);
> >         if (!acquired)
> >                 return 0;
> >
> >         bpf_task_release(acquired);
> >         return 0;
> > }
> >
> > To address this, this patch defines a new type flag called PTR_NESTED
> > which tracks whether a PTR_TO_BTF_ID pointer was retrieved from walking
> > a struct. A pointer passed directly from the kernel begins with
> > (PTR_NESTED & type) == 0, meaning of course that it is not nested. Any
> > pointer received from walking that object, however, would inherit that
> > flag and become a nested pointer.
> >
> > With that flag, this patch also updates btf_check_func_arg_match() to
> > only flag a PTR_TO_BTF_ID object as requiring a refcount if it has any
> > type modifiers (which of course includes both PTR_UNTRUSTED and
> > PTR_NESTED). Otherwise, the pointer passes this check and continues
> > onto the others in btf_check_func_arg_match().
> >
> > A subsequent patch will add kfuncs for storing a task kfunc as a kptr,
> > and then another patch will validate this feature by ensuring that the
> > verifier rejects a kfunc invocation with a nested pointer.
> >
> > Signed-off-by: David Vernet <void@manifault.com>
> > ---
> >  include/linux/bpf.h                          |  6 ++++++
> >  kernel/bpf/btf.c                             | 11 ++++++++++-
> >  kernel/bpf/verifier.c                        | 12 +++++++++++-
> >  tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
> >  4 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 9e7d46d16032..b624024edb4e 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -457,6 +457,12 @@ enum bpf_type_flag {
> >         /* Size is known at compile time. */
> >         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
> >
> > +       /* PTR was obtained from walking a struct. This is used with
> > +        * PTR_TO_BTF_ID to determine whether the pointer is safe to pass to a
> > +        * kfunc with KF_TRUSTED_ARGS.
> > +        */
> > +       PTR_NESTED              = BIT(11 + BPF_BASE_TYPE_BITS),
> > +
> >         __BPF_TYPE_FLAG_MAX,
> >         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
> >  };
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index eba603cec2c5..3d7bad11b10b 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6333,8 +6333,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> >                 /* Check if argument must be a referenced pointer, args + i has
> >                  * been verified to be a pointer (after skipping modifiers).
> >                  * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> > +                *
> > +                * All object pointers must be refcounted, other than:
> > +                * - PTR_TO_CTX
> > +                * - Trusted pointers (i.e. pointers with no type modifiers)
> >                  */
> > -               if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> > +               if (is_kfunc &&
> > +                   trusted_args &&
> > +                   obj_ptr &&
> > +                   base_type(reg->type) != PTR_TO_CTX &&
> > +                   type_flag(reg->type) &&
> > +                   !reg->ref_obj_id) {
> >                         bpf_log(log, "R%d must be referenced\n", regno);
> >                         return -EINVAL;
> >                 }
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 6f6d2d511c06..d16a08ca507b 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -581,6 +581,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> >                 strncpy(prefix, "user_", 32);
> >         if (type & MEM_PERCPU)
> >                 strncpy(prefix, "percpu_", 32);
> > +       if (type & PTR_NESTED)
> > +               strncpy(prefix, "nested_", 32);
> >         if (type & PTR_UNTRUSTED)
> >                 strncpy(prefix, "untrusted_", 32);
> >
> > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> >         if (type_flag(reg->type) & PTR_UNTRUSTED)
> >                 flag |= PTR_UNTRUSTED;
> >
> > +       /* All pointers obtained by walking a struct are nested. */
> > +       flag |= PTR_NESTED;
> > +
> >         if (atype == BPF_READ && value_regno >= 0)
> >                 mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
> >
> > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> >  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> >  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> >  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > +static const struct bpf_reg_types btf_ptr_types = {
> > +       .types = {
> > +               PTR_TO_BTF_ID,
> > +               PTR_TO_BTF_ID | PTR_NESTED
> > +       },
> > +};
> 
> CI fails, two of those failures are from not updating
> check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one

Gah, I didn't think it was necessary for this case as it's not required
for btf_check_func_arg_match(), which will eventually just fail in the
following check:

	if (!btf_type_is_struct(ref_t)) {
		bpf_log(log, "kernel function %s args#%d pointer type %s %s is not support
			func_name, i, btf_type_str(ref_t),
			ref_tname);
		return -EINVAL;
	}

Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
allow it to work properly for normal helper calls, so I'll make that change.
Thanks for pointing it out. In general, the whole dance between register base
types + modifiers sometimes feels like a mine field...

> is a crash because you didn't add to the list in convert_ctx_access
> for PROBE_MEM based load fault handling.

Sorry, just a plain oversight on my end for this one. Thanks for pointing it
out as well.

> The third issue is a bit more interesting:
> 
> ; if (fib6_nh->fib_nh_gw_family) {
> 4375 58: (71) r1 = *(u8 *)(r9 +14)
> 4376 same insn cannot be used with different pointers
> 
> This is because one branch does:
> fib6_nh = &rt->fib6_nh[0];
> while the other does
> fib6_nh = &nh->nh_info->fib6_nh;
> 
> So the load insn 58 in one path uses PTR_TO_BTF_ID and in the other
> PTR_TO_BTF_ID | PTR_NESTED, so reg_type_mismatch bails on seeing src
> != prev.

Ah, that's tricky. Yeah it seems perfectly reasonable to just check the base
type here. As you point out below, check_mem_access() should guard against
invalid accesses.

> I think it's ok to allow this, and we can probably do base_type(src)
> != base_type(prev) here. If the load was not permitted,
> check_mem_access before this should have caught it. The check is
> mostly to prevent doing stuff like PTR_TO_CTX in one path and
> PTR_TO_BTF_ID in the other, because then the convert_ctx_access
> handling would be totally incorrect for the load instruction.
> 
> Hopefully I'm not missing anything.
> 
> But please add some comments about this when updating this particular
> check, it's very easy to forget all this later.

Definitely, will do in v6.

Thanks a lot for the thorough review.

- David
  

On Thu, Oct 20, 2022 at 01:07:09AM IST, David Vernet wrote:
> On Tue, Oct 18, 2022 at 07:02:15AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Please tag the patches with [ PATCH bpf-next ... ] subject prefix.
>
> Sure, will do.
>
> > >  include/linux/bpf.h                          |  6 ++++++
> > >  kernel/bpf/btf.c                             | 11 ++++++++++-
> > >  kernel/bpf/verifier.c                        | 12 +++++++++++-
> > >  tools/testing/selftests/bpf/verifier/calls.c |  4 ++--
> > >  4 files changed, 29 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 9e7d46d16032..b624024edb4e 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -457,6 +457,12 @@ enum bpf_type_flag {
> > >         /* Size is known at compile time. */
> > >         MEM_FIXED_SIZE          = BIT(10 + BPF_BASE_TYPE_BITS),
> > >
> > > +       /* PTR was obtained from walking a struct. This is used with
> > > +        * PTR_TO_BTF_ID to determine whether the pointer is safe to pass to a
> > > +        * kfunc with KF_TRUSTED_ARGS.
> > > +        */
> > > +       PTR_NESTED              = BIT(11 + BPF_BASE_TYPE_BITS),
> > > +
> > >         __BPF_TYPE_FLAG_MAX,
> > >         __BPF_TYPE_LAST_FLAG    = __BPF_TYPE_FLAG_MAX - 1,
> > >  };
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index eba603cec2c5..3d7bad11b10b 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -6333,8 +6333,17 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > >                 /* Check if argument must be a referenced pointer, args + i has
> > >                  * been verified to be a pointer (after skipping modifiers).
> > >                  * PTR_TO_CTX is ok without having non-zero ref_obj_id.
> > > +                *
> > > +                * All object pointers must be refcounted, other than:
> > > +                * - PTR_TO_CTX
> > > +                * - Trusted pointers (i.e. pointers with no type modifiers)
> > >                  */
> > > -               if (is_kfunc && trusted_args && (obj_ptr && reg->type != PTR_TO_CTX) && !reg->ref_obj_id) {
> > > +               if (is_kfunc &&
> > > +                   trusted_args &&
> > > +                   obj_ptr &&
> > > +                   base_type(reg->type) != PTR_TO_CTX &&
> > > +                   type_flag(reg->type) &&
> > > +                   !reg->ref_obj_id) {
> > >                         bpf_log(log, "R%d must be referenced\n", regno);
> > >                         return -EINVAL;
> > >                 }
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 6f6d2d511c06..d16a08ca507b 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -581,6 +581,8 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
> > >                 strncpy(prefix, "user_", 32);
> > >         if (type & MEM_PERCPU)
> > >                 strncpy(prefix, "percpu_", 32);
> > > +       if (type & PTR_NESTED)
> > > +               strncpy(prefix, "nested_", 32);
> > >         if (type & PTR_UNTRUSTED)
> > >                 strncpy(prefix, "untrusted_", 32);
> > >
> >
> > Since these are no longer exclusive, the code needs to be updated to
> > append strings to the prefix buffer.
> > Maybe just using snprintf with %s%s%s.. would be better, passing ""
> > when !(type & flag).
>
> Sure, I can make that change. We'll have to increase the size of the
> prefix string on the stack, but that's hardly problematic as these
> strings are not terribly large.
>
> > > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> > >         if (type_flag(reg->type) & PTR_UNTRUSTED)
> > >                 flag |= PTR_UNTRUSTED;
> > >
> > > +       /* All pointers obtained by walking a struct are nested. */
> > > +       flag |= PTR_NESTED;
> > > +
> >
> > Instead of PTR_NESTED, how about PTR_WALK?
>
> I don't have a strong preference between either, though I would prefer
> PTR_WALKED if we go with the latter. Does that work for you?
>

Yes, I just think PTR_NESTED is a bit misleading, it's not nested within the old
object, we loaded a pointer from it, it should just indicate that the pointer
came from a walk of a trusted PTR_TO_BTF_ID.

> > > [...]
> > > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> > >  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> > >  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> > >  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > > +static const struct bpf_reg_types btf_ptr_types = {
> > > +       .types = {
> > > +               PTR_TO_BTF_ID,
> > > +               PTR_TO_BTF_ID | PTR_NESTED
> > > +       },
> > > +};
> >
> > CI fails, two of those failures are from not updating
> > check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one
>
> Gah, I didn't think it was necessary for this case as it's not required
> for btf_check_func_arg_match(), which will eventually just fail in the
> following check:
>
> 	if (!btf_type_is_struct(ref_t)) {
> 		bpf_log(log, "kernel function %s args#%d pointer type %s %s is not support
> 			func_name, i, btf_type_str(ref_t),
> 			ref_tname);
> 		return -EINVAL;
> 	}

Why would it fail there? It will still be a struct type.
I think you misunderstand this a bit.

When you have task from tracing ctx arg:
r1 = ctx;
r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
// r1 = task->next
r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0

We loaded a pointer from task_struct into r1.
Now r1 still points to a task_struct, so that check above won't fail for r1.

>
> Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> allow it to work properly for normal helper calls, so I'll make that change.
> Thanks for pointing it out. In general, the whole dance between register base
> types + modifiers sometimes feels like a mine field...
>

Yes, I don't like how it's growing and being mixed either. Eventually I think we
should document what combinations are allowed and reject everything else when
initializing reg->type to prevent bugs, but IDK whether something like this
would be accepted.
  

On Thu, Oct 20, 2022 at 11:27:49AM +0530, Kumar Kartikeya Dwivedi wrote:
> > Sure, I can make that change. We'll have to increase the size of the
> > prefix string on the stack, but that's hardly problematic as these
> > strings are not terribly large.
> >
> > > > @@ -4558,6 +4560,9 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> > > >         if (type_flag(reg->type) & PTR_UNTRUSTED)
> > > >                 flag |= PTR_UNTRUSTED;
> > > >
> > > > +       /* All pointers obtained by walking a struct are nested. */
> > > > +       flag |= PTR_NESTED;
> > > > +
> > >
> > > Instead of PTR_NESTED, how about PTR_WALK?
> >
> > I don't have a strong preference between either, though I would prefer
> > PTR_WALKED if we go with the latter. Does that work for you?
> >
> 
> Yes, I just think PTR_NESTED is a bit misleading, it's not nested within the old
> object, we loaded a pointer from it, it should just indicate that the pointer
> came from a walk of a trusted PTR_TO_BTF_ID.

Ok, we'll go with PTR_WALKED.

> > > > [...]
> > > > @@ -5694,7 +5699,12 @@ static const struct bpf_reg_types scalar_types = { .types = { SCALAR_VALUE } };
> > > >  static const struct bpf_reg_types context_types = { .types = { PTR_TO_CTX } };
> > > >  static const struct bpf_reg_types alloc_mem_types = { .types = { PTR_TO_MEM | MEM_ALLOC } };
> > > >  static const struct bpf_reg_types const_map_ptr_types = { .types = { CONST_PTR_TO_MAP } };
> > > > -static const struct bpf_reg_types btf_ptr_types = { .types = { PTR_TO_BTF_ID } };
> > > > +static const struct bpf_reg_types btf_ptr_types = {
> > > > +       .types = {
> > > > +               PTR_TO_BTF_ID,
> > > > +               PTR_TO_BTF_ID | PTR_NESTED
> > > > +       },
> > > > +};
> > >
> > > CI fails, two of those failures are from not updating
> > > check_func_arg_reg_off for PTR_TO_BTF_ID | PTR_WALK, and the other one
> >
> > Gah, I didn't think it was necessary for this case as it's not required
> > for btf_check_func_arg_match(), which will eventually just fail in the
> > following check:
> >
> > 	if (!btf_type_is_struct(ref_t)) {
> > 		bpf_log(log, "kernel function %s args#%d pointer type %s %s is not support
> > 			func_name, i, btf_type_str(ref_t),
> > 			ref_tname);
> > 		return -EINVAL;
> > 	}
> 
> Why would it fail there? It will still be a struct type.
> I think you misunderstand this a bit.

Apologies, as mentioned below I pasted the wrong if-check on accident.
If I had actually meant to paste that one, then saying I "misunderstand
this a bit" would have been a very generous understatment :-)

> When you have task from tracing ctx arg:
> r1 = ctx;
> r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> // r1 = task->next
> r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
> 
> We loaded a pointer from task_struct into r1.
> Now r1 still points to a task_struct, so that check above won't fail for r1.

I meant to paste the if-condition _above_ that one. This is the if-check
we'll fail due to the presence of a type modifier (PTR_WALKED):

	} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
		   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
		const struct btf_type *reg_ref_t;
		const struct btf *reg_btf;
		const char *reg_ref_tname;
		u32 reg_ref_id;

So we'll never even get to the if check I originally pasted because
reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
below we'll eventually fail later on here:

	/* Permit pointer to mem, but only when argument
	 * type is pointer to scalar, or struct composed
	 * (recursively) of scalars.
	 * When arg_mem_size is true, the pointer can be
	 * void *.
	 * Also permit initialized local dynamic pointers.
	 */
	if (!btf_type_is_scalar(ref_t) &&
	    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
	    !arg_dynptr &&
	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
		bpf_log(log,
			"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
			i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
		return -EINVAL;
	}

Appreciate the explanation, sorry to have made you type it.

> > Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> > difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> > allow it to work properly for normal helper calls, so I'll make that change.
> > Thanks for pointing it out. In general, the whole dance between register base
> > types + modifiers sometimes feels like a mine field...
> >
> 
> Yes, I don't like how it's growing and being mixed either. Eventually I think we
> should document what combinations are allowed and reject everything else when
> initializing reg->type to prevent bugs, but IDK whether something like this
> would be accepted.

That seems like a pretty sane idea. A project for another day...
  

On Thu, Oct 20, 2022 at 11:41:41AM IST, David Vernet wrote:
> [...]
> Apologies, as mentioned below I pasted the wrong if-check on accident.
> If I had actually meant to paste that one, then saying I "misunderstand
> this a bit" would have been a very generous understatment :-)
>
> > When you have task from tracing ctx arg:
> > r1 = ctx;
> > r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> > // r1 = task->next
> > r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
> >
> > We loaded a pointer from task_struct into r1.
> > Now r1 still points to a task_struct, so that check above won't fail for r1.
>
> I meant to paste the if-condition _above_ that one. This is the if-check
> we'll fail due to the presence of a type modifier (PTR_WALKED):
>
> 	} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
> 		   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
> 		const struct btf_type *reg_ref_t;
> 		const struct btf *reg_btf;
> 		const char *reg_ref_tname;
> 		u32 reg_ref_id;
>
> So we'll never even get to the if check I originally pasted because
> reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
> below we'll eventually fail later on here:
>
> 	/* Permit pointer to mem, but only when argument
> 	 * type is pointer to scalar, or struct composed
> 	 * (recursively) of scalars.
> 	 * When arg_mem_size is true, the pointer can be
> 	 * void *.
> 	 * Also permit initialized local dynamic pointers.
> 	 */
> 	if (!btf_type_is_scalar(ref_t) &&
> 	    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> 	    !arg_dynptr &&
> 	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> 		bpf_log(log,
> 			"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
> 			i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
> 		return -EINVAL;
> 	}
>
> Appreciate the explanation, sorry to have made you type it.
>

Ah, I see. Your analysis is right, but the error in CI comes from
check_func_arg_reg_off invocation in check_helper_call, this code is for kfuncs.

Since you have this to preserve backwards compat:
+static const struct bpf_reg_types btf_ptr_types = {
+	.types = {
+		PTR_TO_BTF_ID,
+		PTR_TO_BTF_ID | PTR_NESTED
+	},
+};

It allows passing those with PTR_NESTED to stable helpers.
  

On Thu, Oct 20, 2022 at 11:52:29AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Thu, Oct 20, 2022 at 11:41:41AM IST, David Vernet wrote:
> > [...]
> > Apologies, as mentioned below I pasted the wrong if-check on accident.
> > If I had actually meant to paste that one, then saying I "misunderstand
> > this a bit" would have been a very generous understatment :-)
> >
> > > When you have task from tracing ctx arg:
> > > r1 = ctx;
> > > r1 = *(r1 + ...); // PTR_TO_BTF_ID, task_struct, off=0
> > > // r1 = task->next
> > > r1 = *(r1 + offsetof(task_struct, next)); // PTR_TO_BTF_ID | PTR_WALKED, task_struct, off = 0
> > >
> > > We loaded a pointer from task_struct into r1.
> > > Now r1 still points to a task_struct, so that check above won't fail for r1.
> >
> > I meant to paste the if-condition _above_ that one. This is the if-check
> > we'll fail due to the presence of a type modifier (PTR_WALKED):
> >
> > 	} else if (is_kfunc && (reg->type == PTR_TO_BTF_ID ||
> > 		   (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) {
> > 		const struct btf_type *reg_ref_t;
> > 		const struct btf *reg_btf;
> > 		const char *reg_ref_tname;
> > 		u32 reg_ref_id;
> >
> > So we'll never even get to the if check I originally pasted because
> > reg->type == PTR_TO_BTF_ID will fail for a PTR_WALKED reg. And then
> > below we'll eventually fail later on here:
> >
> > 	/* Permit pointer to mem, but only when argument
> > 	 * type is pointer to scalar, or struct composed
> > 	 * (recursively) of scalars.
> > 	 * When arg_mem_size is true, the pointer can be
> > 	 * void *.
> > 	 * Also permit initialized local dynamic pointers.
> > 	 */
> > 	if (!btf_type_is_scalar(ref_t) &&
> > 	    !__btf_type_is_scalar_struct(log, btf, ref_t, 0) &&
> > 	    !arg_dynptr &&
> > 	    (arg_mem_size ? !btf_type_is_void(ref_t) : 1)) {
> > 		bpf_log(log,
> > 			"arg#%d pointer type %s %s must point to %sscalar, or struct with scalar\n",
> > 			i, btf_type_str(ref_t), ref_tname, arg_mem_size ? "void, " : "");
> > 		return -EINVAL;
> > 	}
> >
> > Appreciate the explanation, sorry to have made you type it.
> >
> 
> Ah, I see. Your analysis is right, but the error in CI comes from
> check_func_arg_reg_off invocation in check_helper_call, this code is for kfuncs.

Yeah, in my defense, if you look back at [0] where I fat-fingered the
wrong if statement, I did say that I missed the case for helpers
specifically:

> Note that we also don't include PTR_TO_BTF_ID | PTR_UNTRUSTED here. The
> difference for PTR_TO_BTF_ID | PTR_WALK(ED) is of course that we also need to
> allow it to work properly for normal helper calls, so I'll make that change.

[0]: https://lore.kernel.org/all/Y1BR5c6W4tgljA8q@maniforge.dhcp.thefacebook.com/

Anyways, I think we're on the same page. I already have a local revision
that fixes this.

> Since you have this to preserve backwards compat:
> +static const struct bpf_reg_types btf_ptr_types = {
> +	.types = {
> +		PTR_TO_BTF_ID,
> +		PTR_TO_BTF_ID | PTR_NESTED
> +	},
> +};
> 
> It allows passing those with PTR_NESTED to stable helpers.

I'd need to look over all of this code again to give useful suggestions
here (it's very difficult to keep it all paged in, there's just so much
context), but it'd be nice if we could somehow refactor some of this so
that check_helper_call() and check_kfunc_call() shared more code.
Obviously out of scope for this patch set.