Re: [PATCH RFC v4 2/6] virtio-net: support receive timestamp

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] virtio-net: incomplete TX header zeroing in xmit_skb() leaks skb memory
--

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

virtio-net: support receive timestamp

This commit adds optional hardware RX timestamp offload support via the
VIRTIO_NET_F_RX_TSTAMP feature, extending struct virtio_net_common_hdr to
40 bytes to carry a 64-bit TAI receive timestamp and wiring up the
ethtool and netdev hardware timestamping callbacks.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 17d3e32eed9d6..23af10c613b48 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
[ ... ]
> @@ -3303,6 +3333,10 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb, bool orphan)
>  					false, feature_hdrlen))
>  		return -EPROTO;
>  
> +	/* Don't leak skb->cb or headroom; the device expects tstamp to be 0 */
> +	if (hdr_len > sizeof(*hdr))
> +		memset((u8 *)hdr + sizeof(*hdr), 0, hdr_len - sizeof(*hdr));
> +
>  	if (vi->mergeable_rx_bufs)
>  		hdr->hash_hdr.hdr.num_buffers = 0;

[Severity: Medium]
Can this leave bytes in the middle of the transmitted header uninitialized
and leak stale skb->cb or headroom memory to the device?

When VIRTIO_NET_F_RX_TSTAMP is negotiated, virtnet_probe() sets vi->hdr_len
to 40 bytes (offsetofend(struct virtio_net_common_hdr, tstamp)), even if
hash reporting, UDP tunnel GSO, or mergeable RX buffers were not
negotiated.

In xmit_skb(), hdr has type struct virtio_net_hdr_v1_hash_tunnel *
(sizeof(*hdr) is 24 bytes) and points either into skb headroom or to
&skb_vnet_common_hdr(skb)->tnl_hdr (aliasing skb->cb). When xmit_skb()
calls virtio_net_hdr_tnl_from_skb() for a packet without UDP tunnel GSO, it
returns early after calling virtio_net_hdr_from_skb():

include/linux/virtio_net.h:virtio_net_hdr_tnl_from_skb() {
	tnl_gso_type = skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
						    SKB_GSO_UDP_TUNNEL_CSUM);
	if (!tnl_gso_type) {
		ret = virtio_net_hdr_from_skb(skb, hdr, little_endian,
					      has_data_valid, vlan_hlen);
		if (ret)
			return ret;

		if (feature_hdrlen && hdr->hdr_len)
			__virtio_net_set_hdrlen(skb, hdr, little_endian);

		return ret;
	}
}

Because virtio_net_hdr_from_skb() takes a struct virtio_net_hdr * argument,
its memset(hdr, 0, sizeof(*hdr)) only zeroes the first 10 bytes (offsets
0..9).

The new memset((u8 *)hdr + sizeof(*hdr), 0, hdr_len - sizeof(*hdr)) starts
at offset 24 and zeroes offsets 24..39, while hdr->hash_hdr.hdr.num_buffers
= 0 only zeroes offsets 10..11 when vi->mergeable_rx_bufs is set. As a
result, offsets 12..23 (hash_value_lo, hash_value_hi, hash_report, padding,
outer_th_offset, inner_nh_offset) and offsets 10..11 (when
!vi->mergeable_rx_bufs) are never zeroed before all 40 bytes are mapped into
sq->sg and passed to the device.

Additionally, when !can_push, hdr points to the 24-byte tnl_hdr member
sub-object inside struct virtio_net_common_hdr, so writing past sizeof(*hdr)
via hdr writes across the sub-object boundary into sibling fields of
struct virtio_net_common_hdr.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-v6-7-topic-virtio-net-ptp-v4-0-008af03cd43b@pengutronix.de?part=2