netdev - Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

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, 05 May 2015 12:38:40 -0400
From:	Thomas F Herbert <thomasfherbert@...il.com>
To:	Pravin Shelar <pshelar@...ira.com>
CC:	netdev <netdev@...r.kernel.org>, therbert@...hat.com,
	ccp@...net.net, Flavio Leitner <fbl@...hat.com>
Subject: Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling,
 actions, and vlan parsing

On 4/29/15 9:27 PM, Pravin Shelar wrote:
> On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
> <thomasfherbert@...il.com> wrote:
>
> +
> +       if (likely(skb_vlan_tag_present(skb))) {
> +
> +               key->eth.tci = htons(skb->vlan_tci);
> +
> I think there is bug in existing OVS where it does not check
> vlan_proto before populating the flow key. Can you send a separate
> patch to fix this issue?.
Pravin, I realized I needed to address this  comment in more detail. I 
would appreciate your and anybody
else's thoughts on the following:
1. the newer if_vlan header in the upstream kernel has a function that 
probably should be used here and
elsewhere when checking for tags, skb_vlan_tagged() which also checks 
skb->protocol field.
2.  What about the compat "stuff" in the linux datapath of openvswitch? 
Should any fix for this issue also be
applied to the compat layer which has different semantics for vlans or 
should the compat layer be updated to
be the same as the 3.19, 4.0 kernel semantics?
3.  In spite of my comment #2 above, I am not convinced that the 
generalized skb vlan functions in if_vlan.h are truly
independent of hardware acceleration. I can see cases where the vlan 
headers in the payload of the skb are not
checked. I am thinking a patch to the upstream kernel may also be necessary.
>
>> +               /*
>> +                * Case where upstream
>> +                * processing has already stripped the outer vlan tag.
>> +                */
>> +               if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
>> +
>> +                       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                               return 0;
>> +
> If this returns from here then it can match on flow with tci which
> expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD.
I did not fix this. This double checking has come up before when I 
submitted an earlier revision of this patch.
The double checking done here is also used in the single tagged vlan 
code. I think that the consensus is that
Open vSwitch wants to allow incomplete headers to allow user space 
processing to decide how to
process incomplete vlan tags. For more discussion, see the following thread:
http://openvswitch.org/pipermail/dev/2014-December/049984.html

Thanks,

--TFH

-- 
Thomas F. Herbert

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