Re: [PATCH v2 2/2] vhost/vsock: keep IOTLB across feature updates

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds checking in `vhost_vq_meta_fetch()` combined with a failure to clear the metadata cache during `VHOST_SET_VRING_ADDR` allows malicious userspace to achieve arbitrary kernel read/write via vhost worker threads.
--

--- Patch [2]: [PATCH v2 2/2] vhost/vsock: keep IOTLB across feature updates ---
commit f04e21a46c433140a2f8e01687d07519c6aa6866
Author: Jia Jia <[email protected]>

vhost/vsock: keep IOTLB across feature updates

This commit modifies vhost_vsock_set_features() to only initialize the
device IOTLB when one does not already exist. This prevents dropping
valid translations and forcing avoidable misses when VHOST_SET_FEATURES
is used to update logging while a device is running.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 562b9e139a76f..3703fd219039b 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -893,7 +893,8 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  		goto err;
>  	}
>  
> -	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) {
> +	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
> +	    !vsock->dev.iotlb) {
>  		if (vhost_init_device_iotlb(&vsock->dev))
>  			goto err;
>  	}

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential bounds checking
problem in vhost_vq_meta_fetch() and vhost_vring_set_addr() in
drivers/vhost/vhost.c.

Does vhost_vq_meta_fetch() lack bounds checking on the requested address?

drivers/vhost/vhost.c:vhost_vq_meta_fetch() {
	const struct vhost_iotlb_map *map = vq->meta_iotlb[type];

	if (!map)
		return NULL;

	return (void __user *)(uintptr_t)(map->addr + addr - map->start);
}

When VHOST_SET_VRING_ADDR is called, vhost_vring_set_addr() updates the
vring addresses but does not appear to invalidate the meta_iotlb cache
(for example, by calling __vhost_vq_meta_reset()):

drivers/vhost/vhost.c:vhost_vring_set_addr() {
	...
	vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
	vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
	vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
	vq->log_addr = a.log_guest_addr;
	vq->used = (void __user *)(unsigned long)a.used_user_addr;

	return 0;
}

Could this allow a crafted, out-of-bounds IOVA for the vring to bypass
access_ok() checks and calculate an arbitrary kernel address if a valid
IOTLB mapping was previously established and cached?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2