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

[Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>]

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.


pw-bot: cr