Re: [PATCH v7 2/3] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Concurrent lockless clearing of dev->iotlb causes a TOCTOU race in translate_desc(), leading to a NULL pointer dereference or out-of-bounds memory corruption.
--

--- Patch [2]: [PATCH v7 2/3] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared ---
commit feb57c09c27392564744aed50569fa28daa5a083
Author: Jia Jia <[email protected]>

vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

Clear the device IOTLB when userspace clears VIRTIO_F_ACCESS_PLATFORM.
Otherwise descriptor translation can continue to use mappings installed
before the feature change.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 9aaab6bb8061c..e1e9d002d6aeb 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -865,9 +865,11 @@ 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)) {
>  		if (vhost_init_device_iotlb(&vsock->dev))
>  			goto err;
> +	} else {
> +		vhost_clear_device_iotlb(&vsock->dev);

[Severity: Critical]
Does this concurrent clearing of the IOTLB introduce a regression with
active backend workers?

Looking at vhost_clear_device_iotlb(), it sets d->iotlb to NULL locklessly
before acquiring the vq->mutex:

drivers/vhost/vhost.c:vhost_clear_device_iotlb() {
    ...
    d->iotlb = NULL;

    for (i = 0; i < d->nvqs; ++i) {
        struct vhost_virtqueue *vq = d->vqs[i];
        mutex_lock(&vq->mutex);
    ...
}

If a virtqueue backend worker is actively processing descriptors via
translate_desc() concurrently, it evaluates dev->iotlb without a READ_ONCE():

drivers/vhost/vhost.c:translate_desc() {
    ...
    struct vhost_iotlb *umem = dev->iotlb ? dev->iotlb : dev->umem;
    ...
}

Can the compiler emit multiple fetches here, such that the first fetch is
non-NULL but the second fetch returns NULL? If umem becomes NULL, passing it
to vhost_iotlb_itree_first() could cause a NULL pointer dereference.

Additionally, if the worker sees dev->iotlb as NULL immediately, will it
incorrectly fall back to dev->umem and mistakenly translate the IOVA using
the GPA memory map, leading to memory corruption?

>  	}
>  
>  	vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET);

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