netdev - Re: [RFC PATCH] net act_vlan: use correct len in skb

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: <19f5bdf9-c83b-c9ec-83c3-1e9a88763b30@lab.ntt.co.jp>
Date:   Thu, 14 Feb 2019 18:16:12 +0900
From:   Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
To:     Zahari Doychev <zahari.doychev@...ux.com>, netdev@...r.kernel.org,
        bridge@...ts.linux-foundation.org, nikolay@...ulusnetworks.com,
        roopa@...ulusnetworks.com, jhs@...atatu.com, jiri@...nulli.us,
        xiyou.wangcong@...il.com
Cc:     johannes@...solutions.net
Subject: Re: [RFC PATCH] net act_vlan: use correct len in skb_pull

On 2019/02/14 4:51, Zahari Doychev wrote:
> The bridge and VLAN code expects that skb->data points to the start of the
> VLAN header instead of the next (network) header. Currently after
> tcf_vlan_act() on ingress filter skb->data points to the next network
> header. In this case the Linux bridge does not forward correctly double
> tagged VLAN packets added using tc vlan action as the outer vlan tag from
> the skb is inserted at the wrong offset after the vlan tag in the payload.
> Making skb->data to point to the VLAN header in tcf_vlan_act() by using
> ETH_HLEN in skb_pull_rcsum() fixes the problem.
> 
> The following commands were used for testing:
> 
> ip link add name br0 type bridge vlan_filtering 1
> ip link set dev br0 up
> 
> ip link set dev net0 up
> ip link set dev net0 master br0
> 
> ip link set dev net1 up
> ip link set dev net1 master br0
> 
> bridge vlan add dev net0 vid 100 master
> bridge vlan add dev br0 vid 100 self
> bridge vlan add dev net1 vid 100 master
> 
> tc qdisc add dev net0 handle ffff: clsact
> tc qdisc add dev net1 handle ffff: clsact
> 
> tc filter add dev net0 ingress pref 1 protocol all flower \
>                   action vlan push id 10 pipe action vlan push id 100
> 
> tc filter add dev net0 egress pref 1 protocol 802.1q flower \
>                   vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
>                   action vlan pop pipe action vlan pop
> 
> Signed-off-by: Zahari Doychev <zahari.doychev@...ux.com>
> ---
>  net/sched/act_vlan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 93fdaf707313..308d7d89f925 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -86,7 +86,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
>  
>  out:
>  	if (skb_at_tc_ingress(skb))
> -		skb_pull_rcsum(skb, skb->mac_len);
> +		skb_pull_rcsum(skb, ETH_HLEN);

As I said before, it would be safer to remember mac_len and use it later.

	__u16 mac_len = skb->mac_len;
...
	err = skb_vlan_push(...)
...
	if (skb_at_tc_ingress(skb))
		skb_pull_rcsum(skb, mac_len);


-- 
Toshiaki Makita