Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] vhost-net: incomplete header zeroing in handle_rx() without num_buffers --
commit a70532fad1c69345268d35c3f489b52f8abb6392 Author: Steffen Trumtrar <[email protected]> vhost-net: support VIRTIO_NET_F_RX_TSTAMP This commit advertises VIRTIO_NET_F_RX_TSTAMP in vhost-net, sizes the vnet header to include the timestamp field when negotiated, and zeroes the extra header bytes in handle_rx() when vhost supplies the header itself. > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index 2cc730729e08a..2a38ad0ea1af2 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c [ ... ] > @@ -1282,6 +1283,16 @@ static void handle_rx(struct vhost_net *net) > vhost_discard_vq_desc(vq, headcount, ndesc); > goto out; > } > + /* Zero the rest of a header supplied by vhost, e.g. tstamp */ > + if (unlikely(vhost_hlen > sizeof(hdr) + sizeof(num_buffers))) { > + size_t rest = vhost_hlen - sizeof(hdr) - > sizeof(num_buffers); > + > + if (iov_iter_zero(rest, &fixup) != rest) { > + vq_err(vq, "Failed vnet_hdr zeroing"); > + vhost_discard_vq_desc(vq, headcount, ndesc); > + goto out; > + } > + } [Severity: Medium] Can this leave the final 2 bytes of the vhost-supplied header unzeroed in the guest RX buffer when neither VIRTIO_F_VERSION_1 nor VIRTIO_NET_F_MRG_RXBUF is negotiated? In vhost_net_set_features(), VIRTIO_NET_F_RX_TSTAMP (or UDP tunnel GSO) can be configured alongside VHOST_NET_F_VIRTIO_NET_HDR without requiring VIRTIO_F_VERSION_1 or VIRTIO_NET_F_MRG_RXBUF: drivers/vhost/net.c:vhost_net_set_features() { ... if (virtio_features_test_bit(features, VIRTIO_NET_F_RX_TSTAMP)) hdr_len = offsetofend(struct virtio_net_common_hdr, tstamp); if (virtio_features_test_bit(features, VHOST_NET_F_VIRTIO_NET_HDR)) { /* vhost provides vnet_hdr */ vhost_hlen = hdr_len; ... } When handle_rx() runs with this configuration, set_num_buffers is false and writing num_buffers to fixup is skipped: drivers/vhost/net.c:handle_rx() { ... mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF); set_num_buffers = mergeable || vhost_has_feature(vq, VIRTIO_F_VERSION_1); ... if (likely(set_num_buffers) && copy_to_iter(&num_buffers, sizeof num_buffers, &fixup) != sizeof num_buffers) { ... } Because msg.msg_iter was advanced by vhost_hlen before recvmsg(), the packet payload starts at offset vhost_hlen, while fixup only advanced by sizeof(hdr) (10 bytes). Subtracting sizeof(num_buffers) from rest unconditionally causes iov_iter_zero() to stop 2 bytes short of the payload, leaving the upper 16 bits of tstamp in the guest buffer uninitialized. Should rest only subtract sizeof(num_buffers) when set_num_buffers is true (or should vhost_net_set_features() require VIRTIO_F_VERSION_1 for these extended header features)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261026-v6-7-topic-virtio-net-ptp-v4-0-008af03cd...@pengutronix.de?part=4
