| Message ID | 20230406004018.1439952-2-drosen@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr Verifier Adjustments | expand |
On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <drosen@google.com> wrote: > > This allows using memory retrieved from dynptrs with helper functions > that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data > can be passed along to bpf_strncmp. > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > --- I think patches like this should be targeted against bpf-next, so for next revision please send them against bpf-next tree. > kernel/bpf/verifier.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 56f569811f70..20beab52812a 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > * ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL, > * but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL > * > + * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type. > + * > * Therefore we fold these flags depending on the arg_type before comparison. > */ > if (arg_type & MEM_RDONLY) > type &= ~MEM_RDONLY; > if (arg_type & PTR_MAYBE_NULL) > type &= ~PTR_MAYBE_NULL; > + if (base_type(arg_type) == ARG_PTR_TO_MEM) > + type &= ~DYNPTR_TYPE_FLAG_MASK; Something feels off here. Can you paste a bit of verifier log for the failure you were getting. And let's have a selftest for this situation as well. ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's just memory, there is no need to know what type of dynptr it was derived from. So if that happens, the problem is somewhere else. Let's root cause and fix that. Having a selftest that demonstrates the problem will help with that. > > if (meta->func_id == BPF_FUNC_kptr_xchg && type & MEM_ALLOC) > type &= ~MEM_ALLOC; > -- > 2.40.0.577.gac1e443424-goog >
On Thu, Apr 6, 2023 at 1:55 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Apr 5, 2023 at 5:40 PM Daniel Rosenberg <drosen@google.com> wrote: > > > > This allows using memory retrieved from dynptrs with helper functions > > that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data > > can be passed along to bpf_strncmp. > > > > Signed-off-by: Daniel Rosenberg <drosen@google.com> > > --- > > I think patches like this should be targeted against bpf-next, so for > next revision please send them against bpf-next tree. > > > kernel/bpf/verifier.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 56f569811f70..20beab52812a 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > * ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL, > > * but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL > > * > > + * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type. > > + * > > * Therefore we fold these flags depending on the arg_type before comparison. > > */ > > if (arg_type & MEM_RDONLY) > > type &= ~MEM_RDONLY; > > if (arg_type & PTR_MAYBE_NULL) > > type &= ~PTR_MAYBE_NULL; > > + if (base_type(arg_type) == ARG_PTR_TO_MEM) > > + type &= ~DYNPTR_TYPE_FLAG_MASK; > > Something feels off here. Can you paste a bit of verifier log for the > failure you were getting. And let's have a selftest for this situation > as well. > > ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's > just memory, there is no need to know what type of dynptr it was > derived from. So if that happens, the problem is somewhere else. Let's > root cause and fix that. Having a selftest that demonstrates the > problem will help with that. +1 All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type here. They are either dynamic flags inside bpf_dynptr_kern->size or in arg_type. Like in bpf_dynptr_from_mem_proto.
On Thu, Apr 6, 2023 at 1:55 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > Something feels off here. Can you paste a bit of verifier log for the > failure you were getting. And let's have a selftest for this situation > as well. > > ARG_PTR_TO_MEM shouldn't be qualified with the DYNPTR_TYPE flag, it's > just memory, there is no need to know what type of dynptr it was > derived from. So if that happens, the problem is somewhere else. Let's > root cause and fix that. Having a selftest that demonstrates the > problem will help with that. > > This label is added by dynptr_slice(_rdwr) if (meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice] || meta.func_id == special_kfunc_list[KF_bpf_dynptr_slice_rdwr]) { enum bpf_type_flag type_flag = get_dynptr_type_flag(meta.initialized_dynptr.type); ... regs[BPF_REG_0].type = PTR_TO_MEM | type_flag; That extra flag was causing the type to be unexpected later on. I'll add a selftest for this as well.
On Thu, Apr 6, 2023 at 3:13 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > +1 > All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type > here. > They are either dynamic flags inside bpf_dynptr_kern->size > or in arg_type. > Like in bpf_dynptr_from_mem_proto. Looking at this a bit more, I believe this is to enforce packet restrictions for DYNPTR_TYPE_SKB and DYNPTR_TYPE_XDP. When a helper function alters a packet, dynptr slices of it are invalidated. If I remove that annotation entirely, then the invalid_data_slice family of tests fail. bpf_dynptr_from_mem_proto is fine since that's just local dynptrs, which don't have any extra limitations.
On Mon, May 1, 2023 at 6:12 PM Daniel Rosenberg <drosen@google.com> wrote: > > On Thu, Apr 6, 2023 at 3:13 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > +1 > > All of the DYNPTR_TYPE_FLAG_MASK flags cannot appear in type == reg->type > > here. > > They are either dynamic flags inside bpf_dynptr_kern->size > > or in arg_type. > > Like in bpf_dynptr_from_mem_proto. > > Looking at this a bit more, I believe this is to enforce packet > restrictions for DYNPTR_TYPE_SKB and DYNPTR_TYPE_XDP. When a helper > function alters a packet, dynptr slices of it are invalidated. If I > remove that annotation entirely, then the invalid_data_slice family of > tests fail. bpf_dynptr_from_mem_proto is fine since that's just local > dynptrs, which don't have any extra limitations. Ah, ok, thanks for investigating!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 56f569811f70..20beab52812a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7164,12 +7164,16 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, * ARG_PTR_TO_MEM + MAYBE_NULL is compatible with PTR_TO_MEM and PTR_TO_MEM + MAYBE_NULL, * but ARG_PTR_TO_MEM is compatible only with PTR_TO_MEM but NOT with PTR_TO_MEM + MAYBE_NULL * + * ARG_PTR_TO_MEM is compatible with PTR_TO_MEM that is tagged with a dynptr type. + * * Therefore we fold these flags depending on the arg_type before comparison. */ if (arg_type & MEM_RDONLY) type &= ~MEM_RDONLY; if (arg_type & PTR_MAYBE_NULL) type &= ~PTR_MAYBE_NULL; + if (base_type(arg_type) == ARG_PTR_TO_MEM) + type &= ~DYNPTR_TYPE_FLAG_MASK; if (meta->func_id == BPF_FUNC_kptr_xchg && type & MEM_ALLOC) type &= ~MEM_ALLOC;
This allows using memory retrieved from dynptrs with helper functions that accept ARG_PTR_TO_MEM. For instance, results from bpf_dynptr_data can be passed along to bpf_strncmp. Signed-off-by: Daniel Rosenberg <drosen@google.com> --- kernel/bpf/verifier.c | 4 ++++ 1 file changed, 4 insertions(+)