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