Re: [PATCH] vhost-vdpa: drop the parent's vq callback before the call fd is released

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Torn read causing NULL pointer dereference and incomplete UAF fix for hardware VDPA drivers.
--

--- Patch [1]: [PATCH] vhost-vdpa: drop the parent's vq callback before the call fd is released ---
commit 59fc02f21fdc99bd2ce2389a383eab4badff2c47
Author: Yu Zhang <[email protected]>

vhost-vdpa: drop the parent's vq callback before the call fd is released

This commit clears the vdpa callback pointers before vhost_vring_ioctl()
releases the eventfd to prevent a use-after-free regression with VDUSE. It
aims to ensure the parent driver drops references to the callback context
before it is destroyed.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index c3d913bd7cac7..63ffa94bb4793 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -721,13 +721,34 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
>  			if (ops->get_status(vdpa) &
>  			    VIRTIO_CONFIG_S_DRIVER_OK)
>  				vhost_vdpa_unsetup_vq_irq(v, idx);
> +			/*
> +			 * The parent caches call_ctx.ctx in cb.trigger without
> +			 * holding a reference, so it has to stop using it
> +			 * before vhost_vring_ioctl() drops the last one.
> +			 */
> +			cb.callback = NULL;
> +			cb.private = NULL;
> +			cb.trigger = NULL;
> +			ops->set_vq_cb(vdpa, idx, &cb);

[Severity: Critical]
Can this non-atomic update cause a NULL pointer dereference for hardware
drivers?

For hardware drivers like vp_vdpa, the interrupt handler reads the callback
struct without locks. If a concurrent interrupt occurs during ops->set_vq_cb(),
could it read a torn state where it sees the old valid cb.callback but the
new NULL cb.private?

drivers/vdpa/virtio_pci/vp_vdpa.c:vp_vdpa_vq_handler() {
    ...
	if (vring->cb.callback)
		return vring->cb.callback(vring->cb.private);
    ...
}

If this happens, it would call vhost_vdpa_virtqueue_cb() with a NULL pointer,
causing a panic when attempting to dereference it.

Additionally, since there is no synchronize_irq() here, could an
already-executing hardware interrupt handler still race with the subsequent
vhost_vring_ioctl() freeing the eventfd, leaving the original use-after-free
unfixed for hardware drivers?

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