netdev - Re: [PATCH] vlan: Fix the b0rked 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: <m1aaectyed.fsf@fess.ebiederm.org>
Date:	Mon, 23 May 2011 23:18:02 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	David Miller <davem@...emloft.net>
Cc:	shemminger@...ux-foundation.org, greearb@...delatech.com,
	nicolas.2p.debian@...il.com, jpirko@...hat.com, xiaosuo@...il.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 b0rked ingress VLAN_FLAG_REORDER_HDR check.

David Miller <davem@...emloft.net> writes:

> From: ebiederm@...ssion.com (Eric W. Biederman)
> Date: Mon, 23 May 2011 17:11:51 -0700
>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 41495dc..c08dae7 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -23,6 +23,11 @@ bool vlan_do_receive(struct sk_buff **skbp)
>>  		return false;
>>  
>>  	skb->dev = vlan_dev;
>> +	if (unlikely(!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR))) {
>> +		skb = *skbp = vlan_insert_tag(skb, skb->vlan_tci);
>> +		if (!skb)
>> +			return false;
>> +	}
>>  	skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);
>>  	skb->vlan_tci = 0;
>>  
>
> Below we do a eth_hdr(skb) based check on the ethernet address in the
> PACKET_OTHERHOST case.
>
> Won't this VLAN tag insertion change skb->mac_header and thus screw up
> that test?

Yes the vlan tag insertion does in fact change the skb->mac_header
index.  However we also move the location of the data as well, so
I fail to see any reason to be concerned about the PACKET_OTHERHOST
case.  Things were moved around and we updated the appropriate references.

Feel free to read through the code, to convince yourself it is correct.
In addition the code is untouched from the vlan header insertion for
emulation of vlan header acceleration in dev_hard_start_xmit() which
presumably has been working for quite awhile.

> Touching this code requires surgical precision and long audits, because
> there are so many subtble dependencies all over the place like this.

Certainly.

I think you will find that I have applied great precision and restraint
in this case. 

Eric


--
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