Re: [PATCH bpf-next v4 5/8] bpf: Zero former ARG_PTR_TO_{LONG,INT} args in case of error

[Daniel Borkmann <daniel@iogearbox.net>]

On 9/9/24 2:16 PM, Daniel Borkmann wrote:
> On 9/7/24 12:35 AM, Alexei Starovoitov wrote:
>> On Fri, Sep 6, 2024 at 6:56 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>> -       if (unlikely(flags & ~(BPF_MTU_CHK_SEGS)))
>>> -               return -EINVAL;
>>> -
>>> -       if (unlikely(flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))
>>> +       if (unlikely((flags & ~(BPF_MTU_CHK_SEGS)) ||
>>> +                    (flags & BPF_MTU_CHK_SEGS && (len_diff || *mtu_len)))) {
>>> +               *mtu_len = 0;
>>>                  return -EINVAL;
>>> +       }
>>>
>>>          dev = __dev_via_ifindex(dev, ifindex);
>>> -       if (unlikely(!dev))
>>> +       if (unlikely(!dev)) {
>>> +               *mtu_len = 0;
>>>                  return -ENODEV;
>>> +       }
>>
>> I don't understand this mtu_len clearing.
>>
>> My earlier comment was that mtu is in&out argument.
>> The program has to set it to something. It cannot be uninit.
>> So zeroing it on error looks very odd.
>>
>> In that sense the patch 3 looks wrong. Instead of:
>>
>> @@ -6346,7 +6346,9 @@ static const struct bpf_func_proto
>> bpf_skb_check_mtu_proto = {
>>          .ret_type       = RET_INTEGER,
>>          .arg1_type      = ARG_PTR_TO_CTX,
>>          .arg2_type      = ARG_ANYTHING,
>> -       .arg3_type      = ARG_PTR_TO_INT,
>> +       .arg3_type      = ARG_PTR_TO_FIXED_SIZE_MEM |
>> +                         MEM_UNINIT | MEM_ALIGNED,
>> +       .arg3_size      = sizeof(u32),
>>
>> MEM_UNINIT should be removed, because
>> bpf_xdp_check_mtu, bpf_skb_check_mtu will read it.
>>
>> If there is a program out there that calls this helper without
>> initializing mtu_len it will be rejected after we fix it,
>> but I think that's a good thing.
>> Passing random mtu_len and let helper do:
>>
>> skb_len = *mtu_len ? *mtu_len + dev->hard_header_len : skb->len;
>>
>> is just bad.
> 
> Ok, fair. Removing MEM_UNINIT sounds reasonable, was mostly thinking
> that even if its garbage MTU being passed in, mtu_len gets filled in
> either case (BPF_MTU_CHK_RET_{SUCCESS,FRAG_NEEDED}) assuming no invalid
> ifindex e.g.:
> 
>    __u32 mtu_len;
>    bpf_skb_check_mtu(skb, skb->ifindex, &mtu_len, 0, 0);

Getting back at this, removing MEM_UNINIT needs more verifier rework:
MEM_UNINIT right now implies two things actually: i) write into memory,
ii) memory does not have to be initialized. If we lift MEM_UNINIT, it
then becomes: i) read into memory, ii) memory must be initialized.

This means that for bpf_*_check_mtu() we're readding the issue we're
trying to fix, that is, it would then be able to write back into things
like .rodata maps.

My suggestion is for this series is to go with MEM_UNINIT tag and
clearing on error case to avoid leaking, and then in a subsequent
series to break up MEM_UNINIT in the verifier into two properties:
MEM_WRITE and MEM_UNINIT to better declare intent of the helpers. Then
the bpf_*_check_mtu() will have MEM_WRITE but not MEM_UNINIT.

Thoughts? (If preference is to further extend this series, I can also
look into that ofc.)

Thanks,
Daniel