Re: [PATCH v2 1/2] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

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

Pre-existing issues:
- [Critical] A NULL pointer dereference occurs in translate_desc() if both dev->iotlb and dev->umem are NULL when a descriptor is processed.
--

--- Patch [1]: [PATCH v2 1/2] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared ---
commit 711b531d8532907f465c4af9ee082250c352dc13
Author: Jia Jia <[email protected]>

vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

This patch detaches the IOTLB before acknowledging a feature mask without
ACCESS_PLATFORM to prevent stale translations. It clears the device and
virtqueue IOTLB pointers while holding virtqueue mutexes to ensure kick
handlers do not observe mixed translation states.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 9aaab6bb8061c..562b9e139a76f 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -851,6 +851,34 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
>  	return 0;
>  }
>  
> +/* Caller must hold the device mutex. */
> +static void vhost_vsock_clear_iotlb(struct vhost_vsock *vsock, u64 features)
> +{
> +	struct vhost_iotlb *iotlb;
> +	struct vhost_virtqueue *vq;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++)
> +		mutex_lock_nested(&vsock->vqs[i].mutex, i);
> +
> +	iotlb = vsock->dev.iotlb;
> +	vsock->dev.iotlb = NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
> +		vq = &vsock->vqs[i];
> +		vq->iotlb = NULL;

[Severity: Critical]
This is a pre-existing issue, but by clearing dev->iotlb and vq->iotlb to
NULL here, does this expose a NULL pointer dereference in translate_desc()
if dev->umem was never initialized?

If userspace configures the vrings and kicks the virtqueue without ever
calling VHOST_SET_MEM_TABLE, dev->umem remains NULL. When
vhost_vsock_clear_iotlb() clears dev->iotlb, the next descriptor
translation could trigger this path:

drivers/vhost/vhost.c:translate_desc() {
	...
	struct vhost_iotlb *umem = dev->iotlb ? dev->iotlb : dev->umem;
	...
	while ((u64)len > s) {
		...
		map = vhost_iotlb_itree_first(umem, addr, last);

drivers/vhost/iotlb.c:vhost_iotlb_itree_first() {
	return vhost_iotlb_itree_iter_first(&iotlb->root, start, last);
}

Because umem is NULL, taking the address of iotlb->root passes NULL to the
interval tree iterator, which would cause a page fault when dereferenced.

Can a malicious guest or unprivileged userspace process trigger this by
negotiating VIRTIO_F_ACCESS_PLATFORM, avoiding VHOST_SET_MEM_TABLE, and
then clearing the feature bit dynamically?

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