netdev - Re: [PATCH 1/2] vlan: Fix untag operations of stacked vlans with 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: <566ED772.8070600@6wind.com>
Date:	Mon, 14 Dec 2015 15:51:30 +0100
From:	Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:	Vladislav Yasevich <vyasevich@...il.com>, netdev@...r.kernel.org
Cc:	phil@....cc, kaber@...sh.net,
	Vladislav Yasevich <vyasevic@...hat.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH 1/2] vlan: Fix untag operations of stacked vlans with
 REORDER_HEADER off

Le 16/11/2015 21:43, Vladislav Yasevich a écrit :
> When we have multiple stacked vlan devices all of which have
> turned off REORDER_HEADER flag, the untag operation does not
> locate the ethernet addresses correctly for nested vlans.
> The reason is that in case of REORDER_HEADER flag being off,
> the outer vlan headers are put back and the mac_len is adjusted
> to account for the presense of the header.  Then, the subsequent
> untag operation, for the next level vlan, always use VLAN_ETH_HLEN
> to locate the begining of the ethernet header and that ends up
> being a multiple of 4 bytes short of the actuall beginning
> of the mac header (the multiple depending on the how many vlan
> encapsulations ethere are).
>
> As a reslult, if there are multiple levles of vlan devices
> with REODER_HEADER being off, the recevied packets end up
> being dropped.
>
> To solve this, we use skb->mac_len as the offset.  The value
> is always set on receive path and starts out as a ETH_HLEN.
> The value is also updated when the vlan header manupations occur
> so we know it will be correct.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@...hat.com>
> ---
>   net/core/skbuff.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index fab4599..160193f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4268,7 +4268,8 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>   		return NULL;
>   	}
>
> -	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 2 * ETH_ALEN);
> +	memmove(skb->data - ETH_HLEN, skb->data - skb->mac_len,
> +		2 * ETH_ALEN);
>   	skb->mac_header += VLAN_HLEN;
>   	return skb;
>   }
>
This patch breaks the following test case: a vlan packet is received by an
e1000 interface. Here is the configuration of the interface:
$ ethtool -k ntfp2 | grep "vlan\|offload"
tcp-segmentation-offload: off
udp-fragmentation-offload: off [fixed]
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off [fixed]
rx-vlan-offload: off
tx-vlan-offload: off [fixed]
rx-vlan-filter: on [fixed]
vlan-challenged: off [fixed]
tx-vlan-stag-hw-insert: off [fixed]
rx-vlan-stag-hw-parse: off [fixed]
rx-vlan-stag-filter: off [fixed]
l2-fwd-offload: off [fixed]

The vlan header is not removed by the driver. It calls dev_gro_receive() which
sets the network header to +14, thus mac_len is also sets to 14 and
skb_reorder_vlan_header() do a wrong memmove() (the packet is dropped).
Not sure who is responsible to update mac_len before skb_vlan_untag() is
called. Any suggestions?
--
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