Re: [PATCH RFC v3 2/2] tun: support rx-tstamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: info leak via out-of-bounds stack read in __tun_vnet_hdr_put()
- [High] net: truncation of extended virtio-net headers breaks offloads in __tun_vnet_hdr_get()
- [Medium] net: incorrect endianness when storing virtio-net timestamp in tun_put_user()
--

commit f6ec01e41ac769b584380d489021709a59d0a180
Author: Steffen Trumtrar <s.trumtrar@pengutronix.de>

tun: support rx-tstamp

Demonstrate support for new virtio-net features
VIRTIO_NET_HDR_F_TSTAMP

[ ... ]

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..924784771b6b7 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2157,12 +2157,23 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  	}
>  
>  	if (vnet_hdr_sz) {
> -		struct virtio_net_hdr_v1_hash_tunnel hdr;
> -		struct virtio_net_hdr *gso;
> +		struct virtio_net_common_hdr hdr;
>  
>  		memset(&hdr, 0, sizeof(hdr));
> +
> +		/* hdr has at least the size up to and including the tstamp field. */
> +		if (vnet_hdr_sz >= offsetof(struct virtio_net_common_hdr, tstamp) +
> +				   sizeof_field(struct virtio_net_common_hdr, tstamp)) {
> +			u64 tstamp = ktime_get_clocktai_ns();
> +
> +			hdr.tstamp[0] = (tstamp & 0x000000000000ffffULL) >> 0;
> +			hdr.tstamp[1] = (tstamp & 0x00000000ffff0000ULL) >> 16;
> +			hdr.tstamp[2] = (tstamp & 0x0000ffff00000000ULL) >> 32;
> +			hdr.tstamp[3] = (tstamp & 0xffff000000000000ULL) >> 48;

[Severity: Medium]
Since tstamp is defined as a __virtio16 array, does this need to use
cpu_to_tun_vnet16() for byte swapping?

Assigning a host-endian value directly using native 16-bit shifts could corrupt
the timestamp when read by the guest if the negotiated virtio endianness differs
from the host native endianness.

> +		}
> +
>  		ret = tun_vnet_hdr_tnl_from_skb(tun->flags, tun->dev, skb,

[ ... ]

> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1fa44d..0a7fe1ce889b5 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -122,15 +119,20 @@ static inline int __tun_vnet_hdr_get(int sz, unsigned int flags,
>  				     struct iov_iter *from,
>  				     struct virtio_net_hdr *hdr)
>  {
> -	unsigned int parsed_size = tun_vnet_parse_size(features);
> +	unsigned int parsed_size = tun_vnet_parse_size(sz);
> +	u8 tmp[sizeof(struct virtio_net_common_hdr)]; // temp buffer with known size
>  	u16 hdr_len;
>  
>  	if (iov_iter_count(from) < sz)
>  		return -EINVAL;
>  
> -	if (!copy_from_iter_full(hdr, parsed_size, from))
> +	/* copy parsed size data to the tmp buffer, otherwise compiler will complain */
> +	if (!copy_from_iter_full(tmp, parsed_size, from))
>  		return -EFAULT;
>  
> +	/* now copy the relevant data from tmp to hdr */
> +	memcpy(hdr, tmp, min(parsed_size, sizeof(*hdr)));

[Severity: High]
Does this artificially limit the copy to exactly 10 bytes?

Because hdr is passed as a pointer to struct virtio_net_hdr, sizeof(*hdr)
evaluates statically to 10 bytes.

Callers like tun_get_user() might allocate a larger structure, but they will
only receive 10 bytes, causing extended virtio-net header fields (such as hash
values or tunnel offsets) to be silently dropped.

> +
>  	hdr_len = tun_vnet16_to_cpu(flags, hdr->hdr_len);
>  
>  	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> @@ -155,14 +157,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags,
>  
>  static inline int __tun_vnet_hdr_put(int sz, netdev_features_t features,
>  				     struct iov_iter *iter,
> -				     const struct virtio_net_hdr *hdr)
> +				     const struct virtio_net_common_hdr *hdr)
>  {
> -	unsigned int parsed_size = tun_vnet_parse_size(features);
> +	unsigned int parsed_size = tun_vnet_parse_size(sz);
> +	u8 buf[sizeof(*hdr)];
>  
>  	if (unlikely(iov_iter_count(iter) < sz))
>  		return -EINVAL;
>  
> -	if (unlikely(copy_to_iter(hdr, parsed_size, iter) != parsed_size))
> +	memcpy(buf, hdr, parsed_size);

[Severity: High]
Could this copy leak uninitialized kernel stack memory to userspace?

Callers like tap_put_user() and tun_put_user_xdp() allocate a 10-byte
struct virtio_net_hdr on the kernel stack and pass its pointer to
tun_vnet_hdr_put().

Because tun_vnet_hdr_put() casts the 10-byte pointer to the 32-byte
struct virtio_net_common_hdr:

    return __tun_vnet_hdr_put(sz, 0, iter, (struct virtio_net_common_hdr *)hdr);

and parsed_size can be up to 32 bytes (depending on vnet_hdr_sz), this
memcpy could read 22 bytes past the end of the 10-byte struct.

> +
> +	if (unlikely(copy_to_iter(buf, parsed_size, iter) != parsed_size))
>  		return -EFAULT;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-v6-7-topic-virtio-net-ptp-v3-0-3350851c4f88@pengutronix.de?part=2