| Message ID | 20220912101106.2765921-2-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Allow ringbuf memory to be used as map key | expand |
On Mon, 12 Sept 2022 at 12:24, Dave Marchevsky <davemarchevsky@fb.com> wrote: > > After the previous patch, which added PTR_TO_MEM types to > map_key_value_types, the only difference between map_key_value_types and > mem_types sets is PTR_TO_BUF, which is in the latter set but not the > former. > > Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE > already effectively expect a valid blob of arbitrary memory that isn't > necessarily explicitly associated with a map. When validating a > PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have > already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom > logic like that in process_timer_func or process_kptr_func. > > So let's get rid of map_key_value_types and just use mem_types for those > args. > > This has the effect of adding PTR_TO_BUF to the set of compatible types > for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- I'm wondering how it is safe when PTR_TO_BUF aliases map_value we are updating. User can now do e.g. in array map: map_iter(ctx) bpf_map_update_elem(map, ctx->key, ctx->value, 0); Technically such overlapping memcpy is UB. Looks like for this particular case, overlap will always be exact. Such aliasing pointers always have fixed size memory. If offset was added for partial overlap, it would not satisfy value_size requirement and users won't be able to pass the pointer. dynptr_from_mem may be a problem, but even there you need to obtain a data slice of at least value_size, if an offset is added slice will always be < value_size. So it seems we only need to care about dst == src, and should be using memmove when dst == src? PS: Also it would be better to split verifier and selftest changes in patch 1 into separate patches. > kernel/bpf/verifier.c | 16 ++-------------- > 1 file changed, 2 insertions(+), 14 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index d093618aed99..ae2259d782bb 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5619,18 +5619,6 @@ struct bpf_reg_types { > u32 *btf_id; > }; > > -static const struct bpf_reg_types map_key_value_types = { > - .types = { > - PTR_TO_STACK, > - PTR_TO_PACKET, > - PTR_TO_PACKET_META, > - PTR_TO_MAP_KEY, > - PTR_TO_MAP_VALUE, > - PTR_TO_MEM, > - PTR_TO_MEM | MEM_ALLOC, > - }, > -}; > - > static const struct bpf_reg_types sock_types = { > .types = { > PTR_TO_SOCK_COMMON, > @@ -5691,8 +5679,8 @@ static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE } > static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } }; > > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > - [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > - [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > + [ARG_PTR_TO_MAP_KEY] = &mem_types, > + [ARG_PTR_TO_MAP_VALUE] = &mem_types, > [ARG_CONST_SIZE] = &scalar_types, > [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types, > -- > 2.30.2 >
On Mon, 12 Sept 2022 at 16:34, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Mon, 12 Sept 2022 at 12:24, Dave Marchevsky <davemarchevsky@fb.com> wrote: > > > > After the previous patch, which added PTR_TO_MEM types to > > map_key_value_types, the only difference between map_key_value_types and > > mem_types sets is PTR_TO_BUF, which is in the latter set but not the > > former. > > > > Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE > > already effectively expect a valid blob of arbitrary memory that isn't > > necessarily explicitly associated with a map. When validating a > > PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have > > already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom > > logic like that in process_timer_func or process_kptr_func. > > > > So let's get rid of map_key_value_types and just use mem_types for those > > args. > > > > This has the effect of adding PTR_TO_BUF to the set of compatible types > > for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. > > > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > > --- > > I'm wondering how it is safe when PTR_TO_BUF aliases map_value we are updating. > > User can now do e.g. in array map: > map_iter(ctx) > bpf_map_update_elem(map, ctx->key, ctx->value, 0); > > Technically such overlapping memcpy is UB. Hit send too early. To be fair they can already do this using PTR_TO_MAP_VALUE, so it's not a new problem. > > Looks like for this particular case, overlap will always be exact. > Such aliasing pointers always have fixed size memory. > If offset was added for partial overlap, it would not satisfy > value_size requirement and users won't be able to pass the pointer. > dynptr_from_mem may be a problem, but even there you need to obtain a > data slice of at least value_size, if an offset is added > slice will always be < value_size. > > So it seems we only need to care about dst == src, and should be using > memmove when dst == src? > > PS: Also it would be better to split verifier and selftest changes in > patch 1 into separate patches.
On 9/12/22 11:11 AM, Dave Marchevsky wrote: > After the previous patch, which added PTR_TO_MEM types to > map_key_value_types, the only difference between map_key_value_types and > mem_types sets is PTR_TO_BUF, which is in the latter set but not the > former. Add a selftest with PTR_TO_BUF as the map key/value? > > Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE > already effectively expect a valid blob of arbitrary memory that isn't > necessarily explicitly associated with a map. When validating a > PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have > already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom > logic like that in process_timer_func or process_kptr_func. > > So let's get rid of map_key_value_types and just use mem_types for those > args. > > This has the effect of adding PTR_TO_BUF to the set of compatible types > for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > kernel/bpf/verifier.c | 16 ++-------------- > 1 file changed, 2 insertions(+), 14 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index d093618aed99..ae2259d782bb 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5619,18 +5619,6 @@ struct bpf_reg_types { > u32 *btf_id; > }; > > -static const struct bpf_reg_types map_key_value_types = { > - .types = { > - PTR_TO_STACK, > - PTR_TO_PACKET, > - PTR_TO_PACKET_META, > - PTR_TO_MAP_KEY, > - PTR_TO_MAP_VALUE, > - PTR_TO_MEM, > - PTR_TO_MEM | MEM_ALLOC, > - }, > -}; > - > static const struct bpf_reg_types sock_types = { > .types = { > PTR_TO_SOCK_COMMON, > @@ -5691,8 +5679,8 @@ static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE } > static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } }; > > static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > - [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, > - [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, > + [ARG_PTR_TO_MAP_KEY] = &mem_types, > + [ARG_PTR_TO_MAP_VALUE] = &mem_types, > [ARG_CONST_SIZE] = &scalar_types, > [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, > [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types,
On 9/12/22 3:34 PM, Kumar Kartikeya Dwivedi wrote: > On Mon, 12 Sept 2022 at 16:34, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: >> >> On Mon, 12 Sept 2022 at 12:24, Dave Marchevsky <davemarchevsky@fb.com> wrote: >>> >>> After the previous patch, which added PTR_TO_MEM types to >>> map_key_value_types, the only difference between map_key_value_types and >>> mem_types sets is PTR_TO_BUF, which is in the latter set but not the >>> former. >>> >>> Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE >>> already effectively expect a valid blob of arbitrary memory that isn't >>> necessarily explicitly associated with a map. When validating a >>> PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have >>> already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom >>> logic like that in process_timer_func or process_kptr_func. >>> >>> So let's get rid of map_key_value_types and just use mem_types for those >>> args. >>> >>> This has the effect of adding PTR_TO_BUF to the set of compatible types >>> for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. >>> >>> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> >>> --- >> >> I'm wondering how it is safe when PTR_TO_BUF aliases map_value we are updating. >> >> User can now do e.g. in array map: >> map_iter(ctx) >> bpf_map_update_elem(map, ctx->key, ctx->value, 0); >> >> Technically such overlapping memcpy is UB. > > Hit send too early. > To be fair they can already do this using PTR_TO_MAP_VALUE, so it's > not a new problem. > Yeah, I see what you mean and agree that it's a bug. But as you concluded it's a preexisting issue, so I didn't attempt to address it in the v2 series I sent out today. I agree with all other comments and addressed them. >> >> Looks like for this particular case, overlap will always be exact. >> Such aliasing pointers always have fixed size memory. >> If offset was added for partial overlap, it would not satisfy >> value_size requirement and users won't be able to pass the pointer. >> dynptr_from_mem may be a problem, but even there you need to obtain a >> data slice of at least value_size, if an offset is added >> slice will always be < value_size. >> >> So it seems we only need to care about dst == src, and should be using >> memmove when dst == src? >> >> PS: Also it would be better to split verifier and selftest changes in >> patch 1 into separate patches.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d093618aed99..ae2259d782bb 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5619,18 +5619,6 @@ struct bpf_reg_types { u32 *btf_id; }; -static const struct bpf_reg_types map_key_value_types = { - .types = { - PTR_TO_STACK, - PTR_TO_PACKET, - PTR_TO_PACKET_META, - PTR_TO_MAP_KEY, - PTR_TO_MAP_VALUE, - PTR_TO_MEM, - PTR_TO_MEM | MEM_ALLOC, - }, -}; - static const struct bpf_reg_types sock_types = { .types = { PTR_TO_SOCK_COMMON, @@ -5691,8 +5679,8 @@ static const struct bpf_reg_types timer_types = { .types = { PTR_TO_MAP_VALUE } static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } }; static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { - [ARG_PTR_TO_MAP_KEY] = &map_key_value_types, - [ARG_PTR_TO_MAP_VALUE] = &map_key_value_types, + [ARG_PTR_TO_MAP_KEY] = &mem_types, + [ARG_PTR_TO_MAP_VALUE] = &mem_types, [ARG_CONST_SIZE] = &scalar_types, [ARG_CONST_SIZE_OR_ZERO] = &scalar_types, [ARG_CONST_ALLOC_SIZE_OR_ZERO] = &scalar_types,
After the previous patch, which added PTR_TO_MEM types to map_key_value_types, the only difference between map_key_value_types and mem_types sets is PTR_TO_BUF, which is in the latter set but not the former. Helpers which expect ARG_PTR_TO_MAP_KEY or ARG_PTR_TO_MAP_VALUE already effectively expect a valid blob of arbitrary memory that isn't necessarily explicitly associated with a map. When validating a PTR_TO_MAP_{KEY,VALUE} arg, the verifier expects meta->map_ptr to have already been set, either by an earlier ARG_CONST_MAP_PTR arg, or custom logic like that in process_timer_func or process_kptr_func. So let's get rid of map_key_value_types and just use mem_types for those args. This has the effect of adding PTR_TO_BUF to the set of compatible types for ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- kernel/bpf/verifier.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)