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 <[email protected]> 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-008af03cd...@pengutronix.de?part=2
