netdev - Re: [PATCH RESEND net] net: handle 802.1P vlan 0 packets properly

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]

Date:   Tue, 11 Jun 2019 21:57:50 +0000
From:   "Govindarajulu Varadarajan (gvaradar)" <gvaradar@...co.com>
To:     "Christian Benvenuti (benve)" <benve@...co.com>,
        "toshiaki.makita1@...il.com" <toshiaki.makita1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     "govind.varadar@...il.com" <govind.varadar@...il.com>,
        "ssuryaextr@...il.com" <ssuryaextr@...il.com>
Subject: Re: [PATCH RESEND net] net: handle 802.1P vlan 0 packets properly

@On Tue, 2019-06-11 at 13:34 +0900, Toshiaki Makita wrote:
> On 2019/06/11 3:31, Govindarajulu Varadarajan wrote:
> > When stack receives pkt: [802.1P vlan 0][802.1AD vlan 100][IPv4],
> > vlan_do_receive() returns false if it does not find vlan_dev. Later
> > __netif_receive_skb_core() fails to find packet type handler for
> > skb->protocol 801.1AD and drops the packet.
> > 
> > 801.1P header with vlan id 0 should be handled as untagged packets.
> > This patch fixes it by checking if vlan_id is 0 and processes next vlan
> > header.
> > 
> > Signed-off-by: Govindarajulu Varadarajan <gvaradar@...co.com>
> > ---
> >   net/8021q/vlan_core.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> > index a313165e7a67..0cde54c02c3f 100644
> > --- a/net/8021q/vlan_core.c
> > +++ b/net/8021q/vlan_core.c
> > @@ -9,14 +9,32 @@
> >   bool vlan_do_receive(struct sk_buff **skbp)
> >   {
> >   	struct sk_buff *skb = *skbp;
> > -	__be16 vlan_proto = skb->vlan_proto;
> > -	u16 vlan_id = skb_vlan_tag_get_id(skb);
> > +	__be16 vlan_proto;
> > +	u16 vlan_id;
> >   	struct net_device *vlan_dev;
> >   	struct vlan_pcpu_stats *rx_stats;
> >   
> > +again:
> > +	vlan_proto = skb->vlan_proto;	
> > +	vlan_id = skb_vlan_tag_get_id(skb);
> >   	vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
> > -	if (!vlan_dev)
> > +	if (!vlan_dev) {
> > +		/* Incase of 802.1P header with vlan id 0, continue if
> > +		 * vlan_dev is not found.
> > +		 */
> > +		if (unlikely(!vlan_id)) {
> > +			__vlan_hwaccel_clear_tag(skb);
> 
> Looks like this changes existing behavior. Priority-tagged packets will be
> untagged
> before bridge, etc. I think priority-tagged packets should be forwarded as
> priority-tagged
> (iff bridge is not vlan-aware), not untagged.

Makes sense to me. If rx_handler is registered to the device, pkt should be sent
untagged to rx_handler.

I would like to get some clarification on few more cases before I change the
code. In the setup:

                br0
                 |
  vlan 100       |
   |(802.1AD)    |
   |             |
+--------------------+
|        eth0        |
+--------------------+

Case 1: [802.1P vlan0] [IP]
Current behaviour: pkt is sent to br0 with priority tag. i.e we should not remove
the 802.1P tag.
This patch: removes the 802.1P tag and br0 receives untagged packet. This is
wrong.
Expected behaviour: Should be same as current behaviour.

Case 2: [802.1AD vlan 100] [IP]
Current behaviour: pkt is sent to vlan 100 device.
This patch: same as current behaviour.
Expected behaviour: same as current behaviour

Case 3: [802.1P vlan 0] [802.1AD vlan 100] [IP]
Current behaviour: Pkt is sent to br0 rx_handler. This happens because
vlan_do_receive() returns false (vlan 0 device is not present). Stack does not go
through inner headers.
This patch: pkt is sent to vlan 100 device. Because vlan_do_receive() strips vlan
0 header and finds vlan 100 device.
Expected behaviour: ?
IMO: Pkt should be sent to vlan 100 device because 802.1P should be treated as
priority tag and not as vlan tagged pkt. Since vlan 100 device is present, it
should be sent to vlan 100 device.

Case 4: [802.1AD vlan 200] [802.1AD vlan 100] [IP]
Current behaviour: Pkt is sent to br0 since vlan 200 device is not found.
This patch: same as current behaviour.
Expected behaviour: Same as current behaviour.

Is my understanding correct?