netdev - Re: [PATCH v2 1/4] net: tun: fix tun_xdp_one() for IFF

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: <fedca272-a03e-4bac-4038-2bb1f6b4df84@redhat.com>
Date:   Wed, 23 Jun 2021 11:45:07 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     David Woodhouse <dwmw2@...radead.org>, netdev@...r.kernel.org
Cc:     Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH v2 1/4] net: tun: fix tun_xdp_one() for IFF_TUN mode


在 2021/6/23 上午12:15, David Woodhouse 写道:
> From: David Woodhouse <dwmw@...zon.co.uk>
>
> In tun_get_user(), skb->protocol is either taken from the tun_pi header
> or inferred from the first byte of the packet in IFF_TUN mode, while
> eth_type_trans() is called only in the IFF_TAP mode where the payload
> is expected to be an Ethernet frame.
>
> The alternative path in tun_xdp_one() was unconditionally using
> eth_type_trans(), which corrupts packets in IFF_TUN mode. Fix it to
> do the correct thing for IFF_TUN mode, as tun_get_user() does.
>
> Fixes: 043d222f93ab ("tuntap: accept an array of XDP buffs through sendmsg()")
> Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> ---
>   drivers/net/tun.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 4cf38be26dc9..f812dcdc640e 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2394,8 +2394,50 @@ static int tun_xdp_one(struct tun_struct *tun,
>   		err = -EINVAL;
>   		goto out;
>   	}
> +	switch (tun->flags & TUN_TYPE_MASK) {
> +	case IFF_TUN:
> +		if (tun->flags & IFF_NO_PI) {
> +			u8 ip_version = skb->len ? (skb->data[0] >> 4) : 0;
> +
> +			switch (ip_version) {
> +			case 4:
> +				skb->protocol = htons(ETH_P_IP);
> +				break;
> +			case 6:
> +				skb->protocol = htons(ETH_P_IPV6);
> +				break;
> +			default:
> +				atomic_long_inc(&tun->dev->rx_dropped);
> +				kfree_skb(skb);
> +				err = -EINVAL;
> +				goto out;
> +			}
> +		} else {
> +			struct tun_pi *pi = (struct tun_pi *)skb->data;
> +			if (!pskb_may_pull(skb, sizeof(*pi))) {
> +				atomic_long_inc(&tun->dev->rx_dropped);
> +				kfree_skb(skb);
> +				err = -ENOMEM;
> +				goto out;
> +			}
> +			skb_pull_inline(skb, sizeof(*pi));
> +			skb->protocol = pi->proto;


As replied in previous version, it would be better if we can unify 
similar logic in tun_get_user().

Thanks


> +		}
> +
> +		skb_reset_mac_header(skb);
> +		skb->dev = tun->dev;
> +		break;
> +	case IFF_TAP:
> +		if (!pskb_may_pull(skb, ETH_HLEN)) {
> +			atomic_long_inc(&tun->dev->rx_dropped);
> +			kfree_skb(skb);
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +		skb->protocol = eth_type_trans(skb, tun->dev);
> +		break;
> +	}
>   
> -	skb->protocol = eth_type_trans(skb, tun->dev);
>   	skb_reset_network_header(skb);
>   	skb_probe_transport_header(skb);
>   	skb_record_rx_queue(skb, tfile->queue_index);