Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF_TUN mode

[Jason Wang <jasowang@redhat.com>]

在 2021/6/25 下午4:51, David Woodhouse 写道:
> On Fri, 2021-06-25 at 15:41 +0800, Jason Wang wrote:
>> 在 2021/6/24 下午8:30, David Woodhouse 写道:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> In tun_get_user(), skb->protocol is either taken from the tun_pi header
>>> or inferred from the first byte of the packet in IFF_TUN mode, while
>>> eth_type_trans() is called only in the IFF_TAP mode where the payload
>>> is expected to be an Ethernet frame.
>>>
>>> The equivalent code path in tun_xdp_one() was unconditionally using
>>> eth_type_trans(), which is the wrong thing to do in IFF_TUN mode and
>>> corrupts packets.
>>>
>>> Pull the logic out to a separate tun_skb_set_protocol() function, and
>>> call it from both tun_get_user() and tun_xdp_one().
>>>
>>> XX: It is not entirely clear to me why it's OK to call eth_type_trans()
>>> in some cases without first checking that enough of the Ethernet header
>>> is linearly present by calling pskb_may_pull().
>>
>> Looks like a bug.
>>
>>
>>>     Such a check was never
>>> present in the tun_xdp_one() code path, and commit 96aa1b22bd6bb ("tun:
>>> correct header offsets in napi frags mode") deliberately added it *only*
>>> for the IFF_NAPI_FRAGS mode.
>>
>> We had already checked this in tun_get_user() before:
>>
>>           if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP) {
>>                   align += NET_IP_ALIGN;
>>                   if (unlikely(len < ETH_HLEN ||
>>                                (gso.hdr_len && tun16_to_cpu(tun,
>> gso.hdr_len) < ETH_HLEN)))
>>                           return -EINVAL;
>>           }
> We'd checked skb->len, but that doesn't mean we had a full Ethernet
> header *linearly* at skb->data, does it?


The linear room is guaranteed through either:

1) tun_build_skb()

or

2) tun_alloc_skb()


>
> For the basic tun_get_user() case I suppose we copy_from_user() into a
> single linear skb anyway, even if userspace had fragment it and used
> writev(). So we *are* probably safe there?
>
> I'm sure we *can* contrive a proof that it's safe for that case, if we
> must. But I think we should *need* that proof, if we're going to bypass
> the check. And I wasn't comfortable touching that code without it.
>
> We should also have a fairly good reason... it isn't clear to me *why*
> we're bothering to avoid the check. Is it so slow, even in the case
> where there's nothing to be done?
>
> For a linear skb, the inline pskb_may_pull() is going to immediately
> return true because ETH_HLEN < skb_headlen(skb), isn't it? Why optimise
> *that* away?
>
> Willem, was there a reason you made that conditional in the first
> place?
>
> If we're going to continue to *not* check on the XDP path, we similarly
> need a proof that it can't be fragmented. And also a reason to bother
> with the "optimisation", of course.


For XDP path, we simply need to add a length check since the packet is 
always a linear memory.

Thanks


>
>