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

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

[Yu Zhang]

VHOST_SET_VRING_CALL releases the previous call eventfd inside
vhost_vring_ioctl() -- it swaps the new context into vq->call_ctx.ctx
and then eventfd_ctx_put()s the old one, which is a synchronous
kfree(). The parent vdpa device is only told about the change
afterwards, when vhost_vdpa_vring_ioctl() reaches ops->set_vq_cb().

Parent drivers cache the pointer handed to them in
vdpa_callback::trigger and do not take a reference on it, so
throughout that window the parent holds a dangling eventfd_ctx and may
signal it. The documentation added with the field describes what
signalling it means but says nothing about how long it stays valid.

This is the same hazard that "vhost_vdpa: assign irq bypass producer
token correctly" addressed for the irq bypass producer token, by
moving vhost_vdpa_unsetup_vq_irq() ahead of the vhost_vring_ioctl()
call. The producer token was only one of the two consumers of that
pointer; the one the parent keeps via ->set_vq_cb() was left behind
the free.

With VDUSE the window is directly reachable from userspace, because
the device emulation daemon can inject an interrupt at any time from a
different fd, and neither side shares a lock with the other: VDUSE
takes vq->irq_lock, vhost takes vhost_dev.mutex + vq->mutex.

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0x76/0xe0
  Write of size 4 at addr ffff8881084e8788 by task vduse_uaf/2987

  _raw_spin_lock_irqsave+0x76/0xe0
  eventfd_signal_mask+0x69/0x120
  vduse_dev_ioctl+0x337/0x1a60     <- vduse_vq_signal_irqfd(), inlined
  __x64_sys_ioctl+0x120/0x170      <- VDUSE_VQ_INJECT_IRQ

  Allocated by task 2986:
   do_eventfd+0x50/0x200
   __x64_sys_eventfd2+0x2e/0x40

  kmalloc-64, freed 64-byte region [ffff8881084e8780, ffff8881084e87c0)

One thread loops VHOST_SET_VRING_CALL on /dev/vhost-vdpa-N with a
fresh eventfd and then unbinds it, while another loops
VDUSE_VQ_INJECT_IRQ on /dev/vduse/<name>. This reproduces in 5 out of
5 ten-second runs on v7.1.6 and 3 out of 3 on v7.2-rc6. With the patch
there are no reports in 3 out of 3 runs on either, while the same
workload still gets ~30000 interrupts per run delivered into live
eventfds, so the path is still being exercised.

Tell the parent to drop the callback before vhost_vring_ioctl() can
free the eventfd, mirroring what is already done for the bypass
producer, and restore it if the ioctl fails -- on failure the swap
never happened, the old context is still installed, and leaving the
parent without a callback would silently drop that vq's interrupts.

Fixes: 5e68470f4e80 ("vdpa: Add eventfd for the vdpa callback")
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
 drivers/vhost/vdpa.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ac55275..b7833bd 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -714,13 +714,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);
 		}
 		break;
 	}
 
 	r = vhost_vring_ioctl(&v->vdev, cmd, argp);
-	if (r)
+	if (r) {
+		/*
+		 * A failure here means the swap never happened and the old
+		 * context is still installed, so give the parent back the
+		 * callback that was torn down above.
+		 */
+		if (cmd == VHOST_SET_VRING_CALL && vq->call_ctx.ctx) {
+			cb.callback = vhost_vdpa_virtqueue_cb;
+			cb.private = vq;
+			cb.trigger = vq->call_ctx.ctx;
+			ops->set_vq_cb(vdpa, idx, &cb);
+		}
 		return r;
+	}
 
 	switch (cmd) {
 	case VHOST_SET_VRING_ADDR:
-- 
2.43.0

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

[sashiko-bot]

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 <yuz08559@gmail.com>

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/20260806150323.2154-1-yuz08559@gmail.com?part=1

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

[Michael S. Tsirkin]

On Fri, Aug 07, 2026 at 03:04:04PM +0000, sashiko-bot@kernel.org wrote:
> 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 <yuz08559@gmail.com>
> 
> 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?

Yu Zhang any plans to address these concerns? Seem legit.


> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260806150323.2154-1-yuz08559@gmail.com?part=1
> 

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

[Yu Zhang]

VHOST_SET_VRING_CALL releases the previous call eventfd inside
vhost_vring_ioctl() -- it swaps the new context into vq->call_ctx.ctx
and then eventfd_ctx_put()s the old one, which is a synchronous
kfree(). The parent vdpa device is only told about the change
afterwards, when vhost_vdpa_vring_ioctl() reaches ops->set_vq_cb().

Parent drivers cache the pointer handed to them in
vdpa_callback::trigger and do not take a reference on it, so
throughout that window the parent holds a dangling eventfd_ctx and may
signal it. The documentation added with the field describes what
signalling it means but says nothing about how long it stays valid.

This is the same hazard that "vhost_vdpa: assign irq bypass producer
token correctly" addressed for the irq bypass producer token, by
moving vhost_vdpa_unsetup_vq_irq() ahead of the vhost_vring_ioctl()
call. The producer token was only one of the two consumers of that
pointer; the one the parent keeps via ->set_vq_cb() was left behind
the free.

With VDUSE the window is directly reachable from userspace, because
the device emulation daemon can inject an interrupt at any time from a
different fd, and neither side shares a lock with the other: VDUSE
takes vq->irq_lock, vhost takes vhost_dev.mutex + vq->mutex.

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irqsave+0x76/0xe0
  Write of size 4 at addr ffff8881084e8788 by task vduse_uaf/2987

  _raw_spin_lock_irqsave+0x76/0xe0
  eventfd_signal_mask+0x69/0x120
  vduse_dev_ioctl+0x337/0x1a60     <- vduse_vq_signal_irqfd(), inlined
  __x64_sys_ioctl+0x120/0x170      <- VDUSE_VQ_INJECT_IRQ

  Allocated by task 2986:
   do_eventfd+0x50/0x200
   __x64_sys_eventfd2+0x2e/0x40

  kmalloc-64, freed 64-byte region [ffff8881084e8780, ffff8881084e87c0)

One thread loops VHOST_SET_VRING_CALL on /dev/vhost-vdpa-N with a
fresh eventfd and then unbinds it, while another loops
VDUSE_VQ_INJECT_IRQ on /dev/vduse/<name>. This reproduces in 5 out of
5 ten-second runs on v7.1.6 and 3 out of 3 on v7.2-rc6. With the patch
there are no reports in 3 out of 3 runs on either, while the same
workload still gets ~30000 interrupts per run delivered into live
eventfds, so the path is still being exercised.

Tell the parent to drop the callback before vhost_vring_ioctl() can
free the eventfd, mirroring what is already done for the bypass
producer, and restore it if the ioctl fails -- on failure the swap
never happened, the old context is still installed, and leaving the
parent without a callback would silently drop that vq's interrupts.

Clearing the callback only stops a parent from starting to use the
context; a handler that already loaded it can still be running. For
parents with a real interrupt, wait for it with synchronize_irq() on
the vq's irq. VDUSE does not implement get_vq_irq, so that case stays
covered by the teardown above.

Fixes: 5e68470f4e80 ("vdpa: Add eventfd for the vdpa callback")
Signed-off-by: Yu Zhang <yuz08559@gmail.com>
---
v2, addressing the review on v1:

 - synchronize_irq(): valid, fixed.  Clearing the callback only stops a
   parent from starting to use the context; a handler that already
   loaded it can still be running.  The teardown now ends with a
   synchronize_irq() on ops->get_vq_irq() where the parent has one.
   VDUSE does not implement get_vq_irq, so it is a no-op there and that
   case stays covered by the teardown alone.

 - torn read of the callback struct yielding a NULL cb.private: I do not
   think it applies here.  cb.trigger has exactly one consumer -- apart
   from vhost/vdpa.c assigning it, the only reads are in
   drivers/vdpa/vdpa_user/vduse_dev.c -- and VDUSE serialises both
   sides: vduse_vdpa_set_vq_cb() takes vq->irq_lock around the stores,
   and vduse_vq_irq_inject() and vduse_vq_signal_irqfd() take the same
   lock before using the fields, so VDUSE cannot observe a torn update.
   For parents that do a bare struct copy (vp_vdpa_set_vq_cb() is
   vring->cb = *cb) the hazard is real, but it is not introduced here:
   this function already performs the identical three NULL stores
   through ops->set_vq_cb() in the existing else branch below, which
   userspace reaches with VHOST_FILE_UNBIND.  Defining the store order
   and the matching loads has to happen on the parent side, which this
   caller cannot do; happy to send that separately if you want it.

 - retested on v7.1.6 with KASAN: no reports in 5 out of 5 runs with the
   patch, 3 out of 3 runs report the UAF without it, and the workload is
   not slowed down -- 16k-19k call fd swaps and 1.5M-1.9M injections per
   30s run, against 10k/0.9M measured for v1, with 48k-89k interrupts
   still delivered into live eventfds.

 - rebased on the config_ctx fixes now in mainline.

 drivers/vhost/vdpa.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index a317867..e1def23 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -767,13 +767,46 @@ 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);
+
+			/*
+			 * A parent interrupt handler that loaded the callback
+			 * before that store can still be running, so wait for
+			 * it to finish with the context too.
+			 */
+			if (ops->get_vq_irq) {
+				int irq = ops->get_vq_irq(vdpa, idx);
+
+				if (irq >= 0)
+					synchronize_irq(irq);
+			}
 		}
 		break;
 	}
 
 	r = vhost_vring_ioctl(&v->vdev, cmd, argp);
-	if (r)
+	if (r) {
+		/*
+		 * A failure here means the swap never happened and the old
+		 * context is still installed, so give the parent back the
+		 * callback that was torn down above.
+		 */
+		if (cmd == VHOST_SET_VRING_CALL && vq->call_ctx.ctx) {
+			cb.callback = vhost_vdpa_virtqueue_cb;
+			cb.private = vq;
+			cb.trigger = vq->call_ctx.ctx;
+			ops->set_vq_cb(vdpa, idx, &cb);
+		}
 		return r;
+	}
 
 	switch (cmd) {
 	case VHOST_SET_VRING_ADDR:
-- 
2.43.0

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

[sashiko-bot]

> VHOST_SET_VRING_CALL releases the previous call eventfd inside
> vhost_vring_ioctl() -- it swaps the new context into vq->call_ctx.ctx
> and then eventfd_ctx_put()s the old one, which is a synchronous
> kfree(). The parent vdpa device is only told about the change
> afterwards, when vhost_vdpa_vring_ioctl() reaches ops->set_vq_cb().
> 
> Parent drivers cache the pointer handed to them in
> vdpa_callback::trigger and do not take a reference on it, so
> throughout that window the parent holds a dangling eventfd_ctx and may
> signal it. The documentation added with the field describes what
> signalling it means but says nothing about how long it stays valid.
> 
> This is the same hazard that "vhost_vdpa: assign irq bypass producer
> token correctly" addressed for the irq bypass producer token, by
> moving vhost_vdpa_unsetup_vq_irq() ahead of the vhost_vring_ioctl()
> [ ... ]
> the vq's irq. VDUSE does not implement get_vq_irq, so that case stays
> covered by the teardown above.
> 
> Fixes: 5e68470f4e80 ("vdpa: Add eventfd for the vdpa callback")
> Signed-off-by: Yu Zhang <yuz08559@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918040436.47982-1-yuz08559@gmail.com?part=1