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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);

Will spin a v5 then.

Thanks,
Daniel




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux