netdev - Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <32071ebb3f433b239394e243a6fc8a2bc6d36dcb.camel@infradead.org>
Date:   Fri, 25 Jun 2021 09:51:28 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org
Cc:     Eugenio Pérez <eperezma@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH v3 4/5] net: tun: fix tun_xdp_one() for IFF_TUN mode

On Fri, 2021-06-25 at 15:41 +0800, Jason Wang wrote:
> 在 2021/6/24 下午8:30, David Woodhouse 写道:
> > From: David Woodhouse <dwmw@...zon.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.

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5174 bytes)