On 9/12/22 3:34 PM, Kumar Kartikeya Dwivedi wrote:
> 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.
>
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.
