From 1e1fdcbdde3b7663e5d8faeb2245b9b151417d22 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Mon, 22 Apr 2024 10:03:13 -0400 Subject: vhost/vsock: always initialize seqpacket_allow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are two issues around seqpacket_allow: 1. seqpacket_allow is not initialized when socket is created. Thus if features are never set, it will be read uninitialized. 2. if VIRTIO_VSOCK_F_SEQPACKET is set and then cleared, then seqpacket_allow will not be cleared appropriately (existing apps I know about don't usually do this but it's legal and there's no way to be sure no one relies on this). To fix: - initialize seqpacket_allow after allocation - set it unconditionally in set_features Reported-by: syzbot+6c21aeb59d0e82eb2782@syzkaller.appspotmail.com Reported-by: Jeongjun Park Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport"). Tested-by: Arseny Krasnov Cc: David S. Miller Cc: Stefan Hajnoczi Message-ID: <20240422100010-mutt-send-email-mst@kernel.org> Signed-off-by: Michael S. Tsirkin Acked-by: Jason Wang Reviewed-by: Stefano Garzarella Reviewed-by: Eugenio Pérez Acked-by: Jakub Kicinski --- drivers/vhost/vsock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'drivers/vhost') diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index ec20ecff85c7..bf664ec9341b 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -667,6 +667,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file) } vsock->guest_cid = 0; /* no CID assigned yet */ + vsock->seqpacket_allow = false; atomic_set(&vsock->queued_replies, 0); @@ -810,8 +811,7 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features) goto err; } - if (features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET)) - vsock->seqpacket_allow = true; + vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET); for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { vq = &vsock->vqs[i]; -- cgit v1.2.3 From 7ad4723976672b7d359403a555e0ce5c75529153 Mon Sep 17 00:00:00 2001 From: "Michael S. Tsirkin" Date: Tue, 30 Apr 2024 09:27:48 +1000 Subject: vhost: move smp_rmb() into vhost_get_avail_idx() All callers of vhost_get_avail_idx() use smp_rmb() to order the available ring entry read and avail_idx read. Make vhost_get_avail_idx() call smp_rmb() itself whenever the avail_idx is accessed. This way, the callers don't need to worry about the memory barrier. As a side benefit, we also validate the index on all paths now, which will hopefully help prevent/catch earlier future bugs. Note that current code is inconsistent in how the errors are handled. They are treated as an empty ring in some places, but as non-empty ring in other places. This patch doesn't attempt to change the existing behaviour. No functional change intended. Signed-off-by: Michael S. Tsirkin Reviewed-by: Gavin Shan Acked-by: Will Deacon Message-Id: <20240429232748.642356-1-gshan@redhat.com> --- drivers/vhost/vhost.c | 105 ++++++++++++++++++++------------------------------ 1 file changed, 42 insertions(+), 63 deletions(-) (limited to 'drivers/vhost') diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index b60955682474..9ac25d08f473 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1346,10 +1346,36 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d) mutex_unlock(&d->vqs[i]->mutex); } -static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq, - __virtio16 *idx) +static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq) { - return vhost_get_avail(vq, *idx, &vq->avail->idx); + __virtio16 idx; + int r; + + r = vhost_get_avail(vq, idx, &vq->avail->idx); + if (unlikely(r < 0)) { + vq_err(vq, "Failed to access available index at %p (%d)\n", + &vq->avail->idx, r); + return r; + } + + /* Check it isn't doing very strange thing with available indexes */ + vq->avail_idx = vhost16_to_cpu(vq, idx); + if (unlikely((u16)(vq->avail_idx - vq->last_avail_idx) > vq->num)) { + vq_err(vq, "Invalid available index change from %u to %u", + vq->last_avail_idx, vq->avail_idx); + return -EINVAL; + } + + /* We're done if there is nothing new */ + if (vq->avail_idx == vq->last_avail_idx) + return 0; + + /* + * We updated vq->avail_idx so we need a memory barrier between + * the index read above and the caller reading avail ring entries. + */ + smp_rmb(); + return 1; } static inline int vhost_get_avail_head(struct vhost_virtqueue *vq, @@ -2554,38 +2580,17 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq, { struct vring_desc desc; unsigned int i, head, found = 0; - u16 last_avail_idx; - __virtio16 avail_idx; + u16 last_avail_idx = vq->last_avail_idx; __virtio16 ring_head; int ret, access; - /* Check it isn't doing very strange things with descriptor numbers. */ - last_avail_idx = vq->last_avail_idx; - if (vq->avail_idx == vq->last_avail_idx) { - if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) { - vq_err(vq, "Failed to access avail idx at %p\n", - &vq->avail->idx); - return -EFAULT; - } - vq->avail_idx = vhost16_to_cpu(vq, avail_idx); - - if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) { - vq_err(vq, "Guest moved avail index from %u to %u", - last_avail_idx, vq->avail_idx); - return -EFAULT; - } + ret = vhost_get_avail_idx(vq); + if (unlikely(ret < 0)) + return ret; - /* If there's nothing new since last we looked, return - * invalid. - */ - if (vq->avail_idx == last_avail_idx) + if (!ret) return vq->num; - - /* Only get avail ring entries after they have been - * exposed by guest. - */ - smp_rmb(); } /* Grab the next descriptor number they're advertising, and increment @@ -2846,35 +2851,21 @@ EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n); /* return true if we're sure that avaiable ring is empty */ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq) { - __virtio16 avail_idx; int r; if (vq->avail_idx != vq->last_avail_idx) return false; - r = vhost_get_avail_idx(vq, &avail_idx); - if (unlikely(r)) - return false; - - vq->avail_idx = vhost16_to_cpu(vq, avail_idx); - if (vq->avail_idx != vq->last_avail_idx) { - /* Since we have updated avail_idx, the following - * call to vhost_get_vq_desc() will read available - * ring entries. Make sure that read happens after - * the avail_idx read. - */ - smp_rmb(); - return false; - } + r = vhost_get_avail_idx(vq); - return true; + /* Note: we treat error as non-empty here */ + return r == 0; } EXPORT_SYMBOL_GPL(vhost_vq_avail_empty); /* OK, now we need to know about added descriptors. */ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq) { - __virtio16 avail_idx; int r; if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY)) @@ -2898,25 +2889,13 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq) /* They could have slipped one in as we were doing that: make * sure it's written, then check again. */ smp_mb(); - r = vhost_get_avail_idx(vq, &avail_idx); - if (r) { - vq_err(vq, "Failed to check avail idx at %p: %d\n", - &vq->avail->idx, r); - return false; - } - vq->avail_idx = vhost16_to_cpu(vq, avail_idx); - if (vq->avail_idx != vq->last_avail_idx) { - /* Since we have updated avail_idx, the following - * call to vhost_get_vq_desc() will read available - * ring entries. Make sure that read happens after - * the avail_idx read. - */ - smp_rmb(); - return true; - } + r = vhost_get_avail_idx(vq); + /* Note: we treat error as empty here */ + if (unlikely(r < 0)) + return false; - return false; + return r; } EXPORT_SYMBOL_GPL(vhost_enable_notify); -- cgit v1.2.3 From e400ddf0fb76f451bcf0c7de8a1dbdb1fa0359d5 Mon Sep 17 00:00:00 2001 From: Jeff Johnson Date: Thu, 16 May 2024 18:57:32 -0700 Subject: vringh: add MODULE_DESCRIPTION() Fix the allmodconfig 'make w=1' issue: WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/vhost/vringh.o Signed-off-by: Jeff Johnson Message-Id: <20240516-md-vringh-v1-1-31bf37779a5a@quicinc.com> Signed-off-by: Michael S. Tsirkin Acked-by: Jason Wang --- drivers/vhost/vringh.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/vhost') diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c index 7b8fd977f71c..73e153f9b449 100644 --- a/drivers/vhost/vringh.c +++ b/drivers/vhost/vringh.c @@ -1614,4 +1614,5 @@ EXPORT_SYMBOL(vringh_need_notify_iotlb); #endif +MODULE_DESCRIPTION("host side of a virtio ring"); MODULE_LICENSE("GPL"); -- cgit v1.2.3