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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

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?

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.



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]