On Mon, 12 Sept 2022 at 16:34, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
>
> On Mon, 12 Sept 2022 at 12:24, Dave Marchevsky <davemarchevsky@xxxxxx> 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@xxxxxx>
> > ---
>
> 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.
