Re: [PATCH v2 3/3] virtio: implement synchronize_cbs for remaining transports

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] `virtio_vdpa_synchronize_cbs` fails to synchronize with the configuration callback, bypassing the synchronization mechanism entirely.

Pre-existing issues:
- [High] In remoteproc, virtqueues are freed without waiting for concurrent SRCU readers to finish, violating RCU-001 and leading to a Use-After-Free. Additionally, a TOCTOU vulnerability in `rproc_vq_interrupt` allows a NULL pointer dereference.
- [High] In TmFIFO, virtqueues are freed concurrently with an active background workqueue, leading to Use-After-Free. The synchronization added is inherently flawed and fails to wait for the workqueue.
--

commit 9ddf2954e77ee0736af8a3f1e37367f20723435b
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio: implement synchronize_cbs for remaining transports

virtio_reset_device() now calls synchronize_cbs to wait for running
callbacks. Its synchronize_rcu() fallback does not cover workqueue or
sleepable callbacks.

> diff --git a/drivers/platform/mellanox/mlxbf-tmfifo.c b/drivers/platform/mellanox/mlxbf-tmfifo.c
> index 3c6408581373b..c260e3a1544ea 100644
> --- a/drivers/platform/mellanox/mlxbf-tmfifo.c
> +++ b/drivers/platform/mellanox/mlxbf-tmfifo.c
[ ... ]
> @@ -1135,6 +1135,19 @@ static void mlxbf_tmfifo_virtio_reset(struct virtio_device *vdev)
>  	tm_vdev->status = 0;
>  }
>  
> +static void mlxbf_tmfifo_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct mlxbf_tmfifo_vdev *tm_vdev = mlxbf_vdev_to_tmfifo(vdev);
> +	struct mlxbf_tmfifo *fifo = tm_vdev->vrings[0].fifo;
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fifo->spin_lock); i++) {
> +		spin_lock_irqsave(&fifo->spin_lock[i], flags);
> +		spin_unlock_irqrestore(&fifo->spin_lock[i], flags);
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but does this sequential locking actually
synchronize anything?  Acquiring and releasing locks one by one creates a race
window where callbacks can evade synchronization.

Additionally, does this fail to synchronize with the background workqueue
(fifo->work)?  The workqueue function mlxbf_tmfifo_rxtx_one_desc()
dereferences vring->vq outside of the spinlocks.

When mlxbf_tmfifo_virtio_del_vqs() clears and frees the virtqueue, there is
a use-after-free risk because we haven't flushed the workqueue or properly
synchronized here.

[ ... ]
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index d5e9ff045a28a..ecc022e354db1 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
[ ... ]
> @@ -89,14 +90,19 @@ static bool rproc_virtio_notify(struct virtqueue *vq)
>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
>  {
>  	struct rproc_vring *rvring;
> +	int srcu_idx;
> +	irqreturn_t ret;
> +
> +	srcu_idx = srcu_read_lock(&rproc->vq_srcu);
>  
>  	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
>  
>  	rvring = idr_find(&rproc->notifyids, notifyid);
> -	if (!rvring || !rvring->vq)
> -		return IRQ_NONE;
> +	ret = rvring && rvring->vq ? vring_interrupt(0, rvring->vq) : IRQ_NONE;

[Severity: High]
This is a pre-existing issue, but doesn't this double evaluation of
rvring->vq introduce a Time-of-Check to Time-of-Use (TOCTOU) vulnerability?

Because READ_ONCE() is not used, the compiler could reload a NULL pointer if
__rproc_virtio_del_vqs() clears rvring->vq concurrently, leading to a NULL
pointer dereference when calling vring_interrupt().

> +
> +	srcu_read_unlock(&rproc->vq_srcu, srcu_idx);
>  
> -	return vring_interrupt(0, rvring->vq);
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_vq_interrupt);
>  
[ ... ]
> @@ -242,6 +248,13 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
>  	dev_dbg(&vdev->dev, "reset !\n");
>  }
>  
> +static void rproc_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct rproc *rproc = vdev_to_rproc(vdev);
> +
> +	synchronize_srcu(&rproc->vq_srcu);
> +}

[Severity: High]
This is a pre-existing issue, but virtqueues are freed without waiting for
concurrent SRCU readers to finish, violating the RCU-001 rule (Remove Before
Reclaim).

synchronize_srcu() is called here during device reset, but not during
teardown.  In __rproc_virtio_del_vqs(), the virtqueue is removed and
immediately freed via vring_del_virtqueue() without a grace period.

Since the interrupt handler runs in an SRCU read-side critical section, can
this cause a use-after-free if vring_interrupt() executes on freed memory?

[ ... ]
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index de2af696de6ca..4f9e70c1332ef 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
[ ... ]
> @@ -123,8 +124,24 @@ static irqreturn_t virtio_vdpa_config_cb(void *private)
>  static irqreturn_t virtio_vdpa_virtqueue_cb(void *private)
>  {
>  	struct virtqueue *vq = private;
> +	struct virtio_vdpa_device *vd_dev;
> +	unsigned long flags;
> +	irqreturn_t ret;
>  
> -	return vring_interrupt(0, vq);
> +	vd_dev = to_virtio_vdpa_device(vq->vdev);
> +	read_lock_irqsave(&vd_dev->callback_lock, flags);
> +	ret = vring_interrupt(0, vq);
> +	read_unlock_irqrestore(&vd_dev->callback_lock, flags);
> +
> +	return ret;
> +}

[Severity: Medium]
While virtio_vdpa_virtqueue_cb() was updated to acquire callback_lock,
virtio_vdpa_config_cb() (just above this context in the file) was not.

Doesn't this mean virtio_vdpa_synchronize_cbs() fails to synchronize with
configuration changes, bypassing the synchronization mechanism if a config
change occurs concurrently with device reset or teardown?

> +
> +static void virtio_vdpa_synchronize_cbs(struct virtio_device *vdev)
> +{
> +	struct virtio_vdpa_device *vd_dev = to_virtio_vdpa_device(vdev);
> +
> +	write_lock_irq(&vd_dev->callback_lock);
> +	write_unlock_irq(&vd_dev->callback_lock);
>  }

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