netdev - Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code

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:	Wed, 3 Dec 2014 16:33:57 -0800
From:	Pravin Shelar <pshelar@...ira.com>
To:	Jiri Benc <jbenc@...hat.com>
Cc:	Jiri Pirko <jiri@...nulli.us>, netdev <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Tom Herbert <therbert@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Willem de Bruijn <willemb@...gle.com>,
	Daniel Borkmann <dborkman@...hat.com>, mst@...hat.com,
	fw@...len.de, Paul.Durrant@...rix.com, Thomas Graf <tgraf@...g.ch>,
	Cong Wang <cwang@...pensource.com>
Subject: Re: [patch net-next v4 8/9] net: move vlan pop/push functions into
 common code

On Fri, Nov 21, 2014 at 11:29 AM, Jiri Benc <jbenc@...hat.com> wrote:
> On Fri, 21 Nov 2014 10:41:27 -0800, Pravin Shelar wrote:
>> There is bug in the openvswitch code where vlan-pop does mac-length
>> reset rather than subtracting VLAN_HLEN  from mac-len. MPLS code
>> depends on this. Now this patch moves the bug to common code. I am
>> fine with this patch and I can send fix to change code vlan-pop code
>> to adjust mac_len later on.
>> But I am worried about the comment made by Jiri Benc on earlier
>> version of this patch where is mentioned that subtracting VLAN_HLEN
>> can break common case for vlan-pop. In that case we can not change
>> this common function. Jiri Benc, Can you point me to the code, so the
>> we can work on solution to fix this issue.
>
> In such case, a skb that enters the __pop_vlan_tci function looks like
> this:
>
> +--------+--------+-------------+-----+---------+-------
> | dstmac | srcmac | ETH_P_8021Q | tci | ETH_P_x | data
> +--------+--------+-------------+-----+---------+-------
> ^                               ^
> mac_header                      network_header
>
> skb->protocol = ETH_P_8021Q
> skb->mac_len = 14
>
> After the function finishes, it's supposed to look like this:
>
>                     +--------+--------+---------+-------
>                     | dstmac | srcmac | ETH_P_x | data
>                     +--------+--------+---------+-------
>                     ^                           ^
>                     mac_header                  network_header
>
> skb->protocol = ETH_P_x
> skb->mac_len = 14
>
> Blindly decrementing mac_len wouldn't work here, you'd end up with
> mac_len = 10. You'd either need to ensure this case doesn't happen (it
> does with openvswitch currently) or detect this case.
>

OVS correctly sets mac header length in case of vlan header. Can you
give me OVS test case to reproduce this issue?
--
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