Re: [PATCH net] tun: zero the whole vnet header in tun_put_user()

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

Xiang Mei wrote:
> tun_put_user() declares an on-stack struct virtio_net_hdr_v1_hash_tunnel
> without zeroing it. For a non-tunnel skb, virtio_net_hdr_tnl_from_skb()
> only initializes the first 10 bytes (sizeof(struct virtio_net_hdr)),
> leaving bytes 10..23 (num_buffers and the hash/tunnel fields) as stack
> garbage.
> 
> An unprivileged user can set the vnet header size to 24 with
> TUNSETVNETHDRSZ, so __tun_vnet_hdr_put() copies all 24 bytes of the
> partially-initialized struct to userspace, leaking 14 bytes of kernel
> stack on every read of a non-tunnel packet.
>
> Fix it the same way tun_get_user() already does by zeroing the whole
> header right after declaration.
> 
> Fixes: 288f30435132 ("tun: enable gso over UDP tunnel support.")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Xiang Mei <xmei5@asu.edu>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  drivers/net/tun.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 9e7744eb57a3..fed9dfdfcc3b 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2070,6 +2070,7 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  		struct virtio_net_hdr_v1_hash_tunnel hdr;
>  		struct virtio_net_hdr *gso;
>  
> +		memset(&hdr, 0, sizeof(hdr));

Alternatively clear the trailing bytes only when uninitialized in
virtio_net_hdr_tnl_from_skb. Sketch:

"
+++ b/include/linux/virtio_net.h
@@ -437,6 +437,7 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
                if (feature_hdrlen && hdr->hdr_len)
                        __virtio_net_set_hdrlen(skb, hdr, little_endian);
 
+               memset(hdr + 1, 0, sizeof(*vhdr) - sizeof(*hdr));
                return ret;
        }
"

But it's not trivial to very that all fields beyond the basic header
do get initialized in the tunnel case. So clearing entirely certainly
is a more straightforward correctness analysis.

>  		ret = tun_vnet_hdr_tnl_from_skb(tun->flags, tun->dev, skb,
>  						&hdr);
>  		if (ret)
> -- 
> 2.43.0
>