On Thu, Sep 12, 2024 at 2:47 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx> 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.)
We have MEM_RDONLY which literally means that memory is meant to be
only consumed for reading. Will this solve these issues?
>
> Thanks,
> Daniel
