netdev - Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices

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]

Message-ID: <135ffa7a-f06a-80e3-4412-17457b202c77@gmail.com>
Date:   Fri, 14 Feb 2020 09:56:42 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>, davem@...emloft.net,
        netdev@...r.kernel.org
Cc:     Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH v2 net 3/3] wireguard: send: account for mtu=0 devices



On 2/14/20 9:34 AM, Jason A. Donenfeld wrote:
> It turns out there's an easy way to get packets queued up while still
> having an MTU of zero, and that's via persistent keep alive. This commit
> makes sure that in whatever condition, we don't wind up dividing by
> zero. Note that an MTU of zero for a wireguard interface is something
> quasi-valid, so I don't think the correct fix is to limit it via
> min_mtu. This can be reproduced easily with:
> 
> ip link add wg0 type wireguard
> ip link add wg1 type wireguard
> ip link set wg0 up mtu 0
> ip link set wg1 up
> wg set wg0 private-key <(wg genkey)
> wg set wg1 listen-port 1 private-key <(wg genkey) peer $(wg show wg0 public-key)
> wg set wg0 peer $(wg show wg1 public-key) persistent-keepalive 1 endpoint 127.0.0.1:1
> 
> However, while min_mtu=0 seems fine, it makes sense to restrict the
> max_mtu. This commit also restricts the maximum MTU to the greatest
> number for which rounding up to the padding multiple won't overflow a
> signed integer. Packets this large were always rejected anyway
> eventually, due to checks deeper in, but it seems more sound not to even
> let the administrator configure something that won't work anyway.
> 
> We use this opportunity to clean up this function a bit so that it's
> clear which paths we're expecting.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> Cc: Eric Dumazet <edumazet@...gle.com>
> ---
>  drivers/net/wireguard/device.c |  7 ++++---
>  drivers/net/wireguard/send.c   | 16 +++++++++++-----
>  2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
> index 43db442b1373..cdc96968b0f4 100644
> --- a/drivers/net/wireguard/device.c
> +++ b/drivers/net/wireguard/device.c
> @@ -258,6 +258,8 @@ static void wg_setup(struct net_device *dev)
>  	enum { WG_NETDEV_FEATURES = NETIF_F_HW_CSUM | NETIF_F_RXCSUM |
>  				    NETIF_F_SG | NETIF_F_GSO |
>  				    NETIF_F_GSO_SOFTWARE | NETIF_F_HIGHDMA };
> +	const int overhead = MESSAGE_MINIMUM_LENGTH + sizeof(struct udphdr) +
> +			     max(sizeof(struct ipv6hdr), sizeof(struct iphdr));
>  
>  	dev->netdev_ops = &netdev_ops;
>  	dev->hard_header_len = 0;
> @@ -271,9 +273,8 @@ static void wg_setup(struct net_device *dev)
>  	dev->features |= WG_NETDEV_FEATURES;
>  	dev->hw_features |= WG_NETDEV_FEATURES;
>  	dev->hw_enc_features |= WG_NETDEV_FEATURES;
> -	dev->mtu = ETH_DATA_LEN - MESSAGE_MINIMUM_LENGTH -
> -		   sizeof(struct udphdr) -
> -		   max(sizeof(struct ipv6hdr), sizeof(struct iphdr));
> +	dev->mtu = ETH_DATA_LEN - overhead;
> +	dev->max_mtu = round_down(INT_MAX, MESSAGE_PADDING_MULTIPLE) - overhead;
>  
>  	SET_NETDEV_DEVTYPE(dev, &device_type);
>  
> diff --git a/drivers/net/wireguard/send.c b/drivers/net/wireguard/send.c
> index c13260563446..2a9990ab66cd 100644
> --- a/drivers/net/wireguard/send.c
> +++ b/drivers/net/wireguard/send.c
> @@ -143,16 +143,22 @@ static void keep_key_fresh(struct wg_peer *peer)
>  
>  static unsigned int calculate_skb_padding(struct sk_buff *skb)
>  {
> +	unsigned int padded_size, last_unit = skb->len;
> +
> +	if (unlikely(!PACKET_CB(skb)->mtu))
> +		return -last_unit % MESSAGE_PADDING_MULTIPLE;

My brain hurts.

> +
>  	/* We do this modulo business with the MTU, just in case the networking
>  	 * layer gives us a packet that's bigger than the MTU. In that case, we
>  	 * wouldn't want the final subtraction to overflow in the case of the
> -	 * padded_size being clamped.
> +	 * padded_size being clamped. Fortunately, that's very rarely the case,
> +	 * so we optimize for that not happening.
>  	 */
> -	unsigned int last_unit = skb->len % PACKET_CB(skb)->mtu;
> -	unsigned int padded_size = ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE);
> +	if (unlikely(last_unit > PACKET_CB(skb)->mtu))
> +		last_unit %= PACKET_CB(skb)->mtu;
>  
> -	if (padded_size > PACKET_CB(skb)->mtu)
> -		padded_size = PACKET_CB(skb)->mtu;
> +	padded_size = min(PACKET_CB(skb)->mtu,
> +			  ALIGN(last_unit, MESSAGE_PADDING_MULTIPLE));
>  	return padded_size - last_unit;
>  }
>  


Oh dear, can you describe what do you expect of a wireguard device with mtu == 0 or mtu == 1

Why simply not allowing silly configurations, instead of convoluted tests in fast path ?

We are speaking of tunnels adding quite a lot of headers, so we better not try to make them
work on networks with tiny mtu. Just say no to syzbot.