netdev - Re: [PATCH net 2/2] virtio_net: check L3 protocol for VLAN packets

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: <Yl4pG8MN7jxVybPB@Laptop-X1>
Date:   Tue, 19 Apr 2022 11:14:35 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Maxim Mikityanskiy <maximmi@...lanox.com>,
        Mike Pattrick <mpattric@...hat.com>,
        "Michael S . Tsirkin" <mst@...hat.com>, netdev@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        virtualization@...ts.linux-foundation.org,
        Balazs Nemeth <bnemeth@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net 2/2] virtio_net: check L3 protocol for VLAN packets

On Mon, Apr 18, 2022 at 11:40:44AM -0400, Willem de Bruijn wrote:
> On Mon, Apr 18, 2022 at 12:44 AM Hangbin Liu <liuhangbin@...il.com> wrote:
> >
> > For gso packets, virtio_net_hdr_to_skb() will check the protocol via
> > virtio_net_hdr_match_proto(). But a packet may come from a raw socket
> > with a VLAN tag. Checking the VLAN protocol for virtio net_hdr makes no
> > sense. Let's check the L3 protocol if it's a VLAN packet.
> >
> > Make the virtio_net_hdr_match_proto() checking for all skbs instead of
> > only skb without protocol setting.
> >
> > Also update the data, protocol parameter for
> > skb_flow_dissect_flow_keys_basic() as the skb->protocol may not IP or IPv6.
> >
> > Fixes: 7e5cced9ca84 ("net: accept UFOv6 packages in virtio_net_hdr_to_skb")
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> > ---
> >  include/linux/virtio_net.h | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index a960de68ac69..97b4f9680786 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -3,6 +3,7 @@
> >  #define _LINUX_VIRTIO_NET_H
> >
> >  #include <linux/if_vlan.h>
> > +#include <uapi/linux/if_arp.h>
> >  #include <uapi/linux/tcp.h>
> >  #include <uapi/linux/udp.h>
> >  #include <uapi/linux/virtio_net.h>
> > @@ -102,25 +103,36 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                  */
> >                 if (gso_type && skb->network_header) {
> 
> This whole branch should not be taken by well formed packets. It is
> inside the else clause of
> 
>        if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>           ..
>        } else {
> 
> GSO packets should always request checksum offload. The fact that we
> try to patch up some incomplete packets should not have to be expanded
> if we expand support to include VLAN.

Hi Willem,

I'm not sure if I understand correctly. Do you mean we don't need to check
L3 protocols for VLAN packet without NEEDS_CSUM flag? Which like

if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
	...
} else if (!eth_type_vlan(skb->protocol)) {
	...
}

Thanks
Hangbin