Re: [PATCH net 3/5] net: tun: keep TUN short linear copy hints safe

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jamal Hadi Salim wrote:
> The issue:
> A TUN skb can be redirected by tc mirred to an Ethernet xmit path, which
> calls eth_type_trans() and needs a full Ethernet header in the linear
> area.  tun_get_user() currently validates len and the GSO header length
> only for IFF_TAP, so an IFF_TUN | IFF_VNET_HDR packet can set hdr_len to
> 1..13 and leave fewer than ETH_HLEN linear bytes.  The redirected packet
> then BUGs in eth_type_trans().

TUN mode does not imply Ethernet. Indeed, TUN implies an L3 packet?
> 
> The Fix:
> Reject packets shorter than ETH_HLEN in both modes.  For IFF_TUN, where
> hdr_len is only an allocation/copy hint when GSO and checksum offload are
> off, raise a short nonzero hdr_len to ETH_HLEN instead of rejecting the
> write.  Keep the TAP rejection and its NET_IP_ALIGN adjustment unchanged.
> This preserves valid no-offload TUN writes with hints below ETH_HLEN while
> guaranteeing at least ETH_HLEN linear bytes for the redirected consumer.
> 
> The maximum receive headroom advertised through tun_set_headroom() must
> reserve the same linear minimum.  A large upper-device needed_headroom
> (for example an L2 netkit port behind a VLAN in an OVS datapath) otherwise
> caps good_linear below ETH_HLEN and defeats the clamp.  Reserve ETH_HLEN
> for TUN; the TAP branch already reserves ETH_HLEN + NET_IP_ALIGN.
> 
> Conditions to recreate the bug (repro available):
> - create a TUN device with IFF_TUN | IFF_VNET_HDR | IFF_NO_PI;
> - attach ingress mirred egress redirect dev lo;
> - write a virtio_net_hdr with hdr_len=1 and an 8192-byte payload.
> Unfixed kernel will panic in eth_type_trans via loopback_xmit.
> In a fixed kernel the write is accepted with hdr_len clamped to ETH_HLEN and the
> packet is forwarded without a BUG.
> - no-offload acceptance: a separate check writes a 28-byte IPv4/UDP-shaped
>   packet with hdr_len 0, 1, 13, 14 and 20; all writes are accepted.
> - high headroom: create an L2 netkit pair with headroom 4096, put a VLAN
>   over it, and add both the VLAN and the TUN to one OVS datapath so the
>   TUN receive headroom is raised to 4100.  The same hdr_len=1 jumbo must
>   keep at least ETH_HLEN linear bytes; without the setter reservation it
>   still BUGs.
> 
> Fixes: f42157cb568c ("tun: fallback if skb_alloc() fails on big packets")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  drivers/net/tun.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd07..1e235c2f0aee 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1193,7 +1193,7 @@ static void tun_set_headroom(struct net_device *dev, int new_hr)
>  	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
>  		max_headroom -= ETH_HLEN + NET_IP_ALIGN;
>  	else
> -		max_headroom -= 1;
> +		max_headroom -= ETH_HLEN;
>  
>  	tun->align = clamp_t(int, new_hr, NET_SKB_PAD, max_headroom);
>  }
> @@ -1830,6 +1830,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		align += NET_IP_ALIGN;
>  		if (unlikely(len < ETH_HLEN || (hdr_len && hdr_len < ETH_HLEN)))
>  			return -EINVAL;
> +	} else {
> +		if (unlikely(len < ETH_HLEN))
> +			return -EINVAL;
> +		if (hdr_len)
> +			hdr_len = max_t(int, hdr_len, ETH_HLEN);
>  	}
>  
>  	good_linear = SKB_MAX_HEAD(align);
> -- 
> 2.43.0
>