netdev - Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER

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: <m1ipsob6f8.fsf@fess.ebiederm.org>
Date:	Thu, 02 Jun 2011 08:26:51 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Changli Gao <xiaosuo@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	shemminger@...ux-foundation.org, greearb@...delatech.com,
	nicolas.2p.debian@...il.com, jpirko@...hat.com,
	netdev@...r.kernel.org, kaber@...sh.net, fubar@...ibm.com,
	eric.dumazet@...il.com, andy@...yhouse.net, jesse@...ira.com
Subject: Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2

Changli Gao <xiaosuo@...il.com> writes:

> On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@...ssion.com> wrote:
>>
>> -static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
>> +static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
>>  {
>> -       if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
>> -               if (skb_cow(skb, skb_headroom(skb)) < 0)
>> -                       skb = NULL;
>> -               if (skb) {
>> -                       /* Lifted from Gleb's VLAN code... */
>> -                       memmove(skb->data - ETH_HLEN,
>> -                               skb->data - VLAN_ETH_HLEN, 12);
>> -                       skb->mac_header += VLAN_HLEN;
>> -               }
>> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
>> +               skb = NULL;
>> +       if (skb) {
>
> I think an else branch maybe more readable here.

Probably most readable would be simply returning NULL immediately on
error.  In this patch I just removed the if statement, and I would like
to avoid mixing different bug fixes in the same patch if possible.

>> +               /* Lifted from Gleb's VLAN code... */
>> +               memmove(skb->data - ETH_HLEN,
>> +                       skb->data - VLAN_ETH_HLEN, 12);
>> +               skb->mac_header += VLAN_HLEN;
>
> skb->mac_len should be adjusted too.

Given how vlan_untag is called at the moment it does appear
that the skb->mac_len = skb->network_header - skb->mac_header
in __netif_receive_skb that used to handle this for is no longer
doing this for us.

My feel is that either we need to do all of the header resets and the
vlan_untagging together.  So we either need this all together before or
after the another_round label:

So the proper fix is probably something like this.

diff --git a/net/core/dev.c b/net/core/dev.c
index bcb05cb..8fe50d4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3102,9 +3102,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
-	skb_reset_network_header(skb);
-	skb_reset_transport_header(skb);
-	skb->mac_len = skb->network_header - skb->mac_header;
 
 	pt_prev = NULL;
 
@@ -3119,6 +3116,9 @@ another_round:
 		if (unlikely(!skb))
 			goto out;
 	}
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+	skb->mac_len = skb->network_header - skb->mac_header;
 
 #ifdef CONFIG_NET_CLS_ACT
 	if (skb->tc_verd & TC_NCLS) {
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html