Re: [PATCH 2/2] vhost-vdpa: protect config_ctx from being freed under the config callback

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

Pre-existing issues:
- [Critical] An identical Use-After-Free pattern to the one fixed for `v->config_ctx` exists unpatched for `vq->call_ctx.ctx` in the virtqueue interrupt path.
--

--- Patch [2]: [PATCH 2/2] vhost-vdpa: protect config_ctx from being freed under the config callback ---
commit 8e0bc317868b822a05e8a68785677223512b3be4
Author: Yu Zhang <[email protected]>

vhost-vdpa: protect config_ctx from being freed under the config callback

This commit fixes a use-after-free by adding a spinlock to serialize accesses
to config_ctx in the config callback against its replacement in the
VHOST_VDPA_SET_CONFIG_CALL ioctl.

> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index ae137d5be99c9..cae8089ab3ddf 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -194,10 +196,12 @@ static irqreturn_t vhost_vdpa_virtqueue_cb(void *private)

[Severity: Critical]
This is a pre-existing issue, but does an identical use-after-free pattern
exist for vq->call_ctx.ctx in the virtqueue interrupt path?

It appears vhost_vdpa_virtqueue_cb() reads and signals vq->call_ctx.ctx
locklessly:

drivers/vhost/vdpa.c:vhost_vdpa_virtqueue_cb() {
    struct eventfd_ctx *call_ctx = vq->call_ctx.ctx;
    
    if (call_ctx)
        eventfd_signal(call_ctx);
}

Could this race with VHOST_SET_VRING_CALL in vhost_vring_ioctl(), which
swaps out vq->call_ctx.ctx and immediately frees the old context?

drivers/vhost/vhost.c:vhost_vring_ioctl() {
    swap(ctx, vq->call_ctx.ctx);
    ...
    if (!IS_ERR_OR_NULL(ctx))
        eventfd_ctx_put(ctx);
}

If a userspace process with access to /dev/vduse concurrently invokes
VDUSE_VQ_INJECT_IRQ while VHOST_SET_VRING_CALL is executing, could
vduse_vq_signal_irqfd() end up calling eventfd_signal() on freed memory
since it uses the cached vq->cb.trigger?

drivers/vdpa/vdpa_user/vduse_dev.c:vduse_vq_signal_irqfd() {
    if (vq->ready && vq->cb.trigger) {
        eventfd_signal(vq->cb.trigger);
        signal = true;
    }
}

>  static irqreturn_t vhost_vdpa_config_cb(void *private)
>  {
>  	struct vhost_vdpa *v = private;
> -	struct eventfd_ctx *config_ctx = v->config_ctx;
> +	unsigned long flags;
>  
> -	if (config_ctx)
> -		eventfd_signal(config_ctx);
> +	spin_lock_irqsave(&v->config_lock, flags);
> +	if (v->config_ctx)
> +		eventfd_signal(v->config_ctx);
> +	spin_unlock_irqrestore(&v->config_lock, flags);
>  
>  	return IRQ_HANDLED;
>  }

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