netdev - Re: [PATCH] AF_PACKET doesnt strip VLAN information

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: <20191001084427.73f130c0@hermes.lan>
Date:   Tue, 1 Oct 2019 08:44:27 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Sriram Krishnan <srirakr2@...co.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        xe-linux-external@...co.com,
        "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] AF_PACKET doesnt strip VLAN information

On Mon, 30 Sep 2019 11:16:14 -0400
Willem de Bruijn <willemdebruijn.kernel@...il.com> wrote:

> On Mon, Sep 30, 2019 at 1:24 AM Sriram Krishnan <srirakr2@...co.com> wrote:
> >
> > When an application sends with AF_PACKET and places a vlan header on
> > the raw packet; then the AF_PACKET needs to move the tag into the skb
> > so that it gets processed normally through the rest of the transmit
> > path.
> >
> > This is particularly a problem on Hyper-V where the host only allows
> > vlan in the offload info.  
> 
> This sounds like behavior that needs to be addressed in the driver, instead?

This was what we did first, but the problem was more general.
For example, many filtering functions assume that vlan tag is in
skb meta data, not the packet data itself. Therefore AF_PACKET would
get around any filter rules.

> 
> > Cc: xe-linux-external@...co.com
> > ---
> >  net/packet/af_packet.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index e2742b0..cfe0904 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -1849,15 +1849,35 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
> >         return 0;
> >  }
> >
> > -static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
> > +static int packet_parse_headers(struct sk_buff *skb, struct socket *sock)
> >  {
> >         if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
> >             sock->type == SOCK_RAW) {  
> 
> If inside this branch, may miss packets with skb->protocol set to one
> of the VLAN Ethertypes.
> 
> > +               __be16 ethertype;
> > +
> >                 skb_reset_mac_header(skb);
> > +
> > +               ethertype = eth_hdr(skb)->h_proto;
> > +               /*
> > +                * If Vlan tag is present in the packet
> > +                *  move it to skb
> > +               */
> > +               if (eth_type_vlan(ethertype)) {
> > +                       int err;
> > +                       __be16 vlan_tci;
> > +
> > +                       err = __skb_vlan_pop(skb, &vlan_tci);
> > +                       if (unlikely(err))
> > +                               return err;
> > +
> > +                       __vlan_hwaccel_put_tag(skb, ethertype, vlan_tci);  
> 
> What happens with multiple tags (QinQ)?

Same as multiple tags in a normal sent packet. The second tag is in
the packet itself.

> 
> > +               }
> > +
> >                 skb->protocol = dev_parse_header_protocol(skb);
> >         }
> >
> >         skb_probe_transport_header(skb);
> > +       return 0;
> >  }
> >
> >  /*
> > @@ -1979,7 +1999,9 @@ static int packet_sendmsg_spkt(struct socket *sock, struct msghdr *msg,
> >         if (unlikely(extra_len == 4))
> >                 skb->no_fcs = 1;
> >
> > -       packet_parse_headers(skb, sock);
> > +       err = packet_parse_headers(skb, sock);
> > +       if (err)
> > +               goto out_unlock;  
> 
> This only tests the new return value in one of three callers of
> packet_sendmsg_spkt.