[net,stable] tun: fix vlan packet truncation

Message ID 20180416220038.21743-1-bjorn@mork.no
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • [net,stable] tun: fix vlan packet truncation
Related show

Commit Message

Bjørn Mork April 16, 2018, 10 p.m.
Bogus trimming in tun_net_xmit() causes truncated vlan packets.

skb->len is correct whether or not skb_vlan_tag_present() is true. There
is no more reason to adjust the skb length on xmit in this driver than
any other driver. tun_put_user() adds 4 bytes to the total for tagged
packets because it transmits the tag inline to userspace.  This is
similar to a nic transmitting the tag inline on the wire.

Reproducing the bug by sending any tagged packet through back-to-back
connected tap interfaces:

 socat TUN,tun-type=tap,iff-up,tun-name=in TUN,tun-type=tap,iff-up,tun-name=out &
 ip link add link in name in.20 type vlan id 20
 ip addr add 10.9.9.9/24 dev in.20
 ip link set in.20 up
 tshark -nxxi in -f arp -c1 2>/dev/null &
 tshark -nxxi out -f arp -c1 2>/dev/null &
 ping -c 1 10.9.9.5 >/dev/null 2>&1

The output from the 'in' and 'out' interfaces are different when the
bug is present:

 Capturing on 'in'
 0000  ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14   ......v.v7......
 0010  08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a   ..........v.v7..
 0020  0a 09 09 09 00 00 00 00 00 00 0a 09 09 05         ..............

 Capturing on 'out'
 0000  ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14   ......v.v7......
 0010  08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a   ..........v.v7..
 0020  0a 09 09 09 00 00 00 00 00 00                     ..........

Fixes: aff3d70a07ff ("tun: allow to attach ebpf socket filter")
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 drivers/net/tun.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Jason Wang April 17, 2018, 2:13 a.m. | #1
On 2018年04月17日 06:00, Bjørn Mork wrote:
> Bogus trimming in tun_net_xmit() causes truncated vlan packets.
>
> skb->len is correct whether or not skb_vlan_tag_present() is true. There
> is no more reason to adjust the skb length on xmit in this driver than
> any other driver. tun_put_user() adds 4 bytes to the total for tagged
> packets because it transmits the tag inline to userspace.  This is
> similar to a nic transmitting the tag inline on the wire.
>
> Reproducing the bug by sending any tagged packet through back-to-back
> connected tap interfaces:
>
>   socat TUN,tun-type=tap,iff-up,tun-name=in TUN,tun-type=tap,iff-up,tun-name=out &
>   ip link add link in name in.20 type vlan id 20
>   ip addr add 10.9.9.9/24 dev in.20
>   ip link set in.20 up
>   tshark -nxxi in -f arp -c1 2>/dev/null &
>   tshark -nxxi out -f arp -c1 2>/dev/null &
>   ping -c 1 10.9.9.5 >/dev/null 2>&1
>
> The output from the 'in' and 'out' interfaces are different when the
> bug is present:
>
>   Capturing on 'in'
>   0000  ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14   ......v.v7......
>   0010  08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a   ..........v.v7..
>   0020  0a 09 09 09 00 00 00 00 00 00 0a 09 09 05         ..............
>
>   Capturing on 'out'
>   0000  ff ff ff ff ff ff 76 cf 76 37 d5 0a 81 00 00 14   ......v.v7......
>   0010  08 06 00 01 08 00 06 04 00 01 76 cf 76 37 d5 0a   ..........v.v7..
>   0020  0a 09 09 09 00 00 00 00 00 00                     ..........
>
> Fixes: aff3d70a07ff ("tun: allow to attach ebpf socket filter")
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>   drivers/net/tun.c | 7 -------
>   1 file changed, 7 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 28583aa0c17d..01cf8e3d8edc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1103,13 +1103,6 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>   
>   	len = run_ebpf_filter(tun, skb, len);
>   
> -	/* Trim extra bytes since we may insert vlan proto & TCI
> -	 * in tun_put_user().
> -	 */
> -	len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0;
> -	if (len <= 0 || pskb_trim(skb, len))
> -		goto drop;
> -
>   	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
>   		goto drop;
>   

Good catch, but I still think we should do the truncation in 
run_ebpf_filter to match the behavior socket ebpf filter.

Thanks
Bjørn Mork April 17, 2018, 8:08 a.m. | #2
Jason Wang <jasowang@redhat.com> writes:

> Good catch,

Thanks.  I guess I am "lucky" as I apparently use configs no one else
are using, and therefore often experience breakage ;-)  

I have been using tagged interfaces over tap for an occasional virtual
"lab" of kvm machines for years. It's a simple way to have a flexible
netowrk between host and virtual machine.  So v4.16 broke my "lab". I'm
sorry it took me a while before I had time to investigate why.

> but I still think we should do the truncation in
> run_ebpf_filter to match the behavior socket ebpf filter.

Haven't looked at that code.  But I'd like to point out that any length
adjustments should be done next to the code adding or removing data.
Doing data and length modifications in different funtions is either a
bug, or a bug bound to happen.



Bjørn
David Miller April 17, 2018, 5:47 p.m. | #3
From: Bjørn Mork <bjorn@mork.no>
Date: Tue, 17 Apr 2018 00:00:38 +0200

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 28583aa0c17d..01cf8e3d8edc 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1103,13 +1103,6 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	len = run_ebpf_filter(tun, skb, len);
>  
> -	/* Trim extra bytes since we may insert vlan proto & TCI
> -	 * in tun_put_user().
> -	 */
> -	len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0;
> -	if (len <= 0 || pskb_trim(skb, len))
> -		goto drop;
> -
>  	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))

The VLAN business might be bogus, and needs to be removed.

However, the pskb_trim() has to stay in some form.  I think this is what
Jason is trying to say.

The semantics of running a BPF program is that the program returns the
desired packet length.  We must truncate the packet to the length
returned by the BPF program, therefore.
Bjørn Mork April 17, 2018, 8:41 p.m. | #4
David Miller <davem@davemloft.net> writes:

> The VLAN business might be bogus, and needs to be removed.
>
> However, the pskb_trim() has to stay in some form.  I think this is what
> Jason is trying to say.
>
> The semantics of running a BPF program is that the program returns the
> desired packet length.  We must truncate the packet to the length
> returned by the BPF program, therefore.

Right. That's obvious when I look closer at it. Sorry for being so
slow...  New version coming up.


Bjørn

Patch

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 28583aa0c17d..01cf8e3d8edc 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1103,13 +1103,6 @@  static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	len = run_ebpf_filter(tun, skb, len);
 
-	/* Trim extra bytes since we may insert vlan proto & TCI
-	 * in tun_put_user().
-	 */
-	len -= skb_vlan_tag_present(skb) ? sizeof(struct veth) : 0;
-	if (len <= 0 || pskb_trim(skb, len))
-		goto drop;
-
 	if (unlikely(skb_orphan_frags_rx(skb, GFP_ATOMIC)))
 		goto drop;