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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 5 May 2015 16:27:27 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Thomas F Herbert <thomasfherbert@...il.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 Tue, May 5, 2015 at 9:38 AM, Thomas F Herbert
<thomasfherbert@...il.com> wrote:
> 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.
When we populate flow key, we need to check skb->vlan_proto if it is
8021Q. This bug fix should be targeted to net tree.

> 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?
I think compat layer is fine. But if you find any issue you can send fix.

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

ok.

>>
>>
>>> +               /*
>>> +                * 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
>
When you return from error case you need to clear the key->eth.tci, so
that it does not match on wrong flow.
--
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