Re: [PATCH RFC v4 4/6] vhost-net: support VIRTIO_NET_F_RX_TSTAMP

[sashiko-bot@kernel.org]

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 <s.trumtrar@pengutronix.de>

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-008af03cd43b@pengutronix.de?part=4