Hi,
Could someone help to review it or provide suggestions?
Any comments are welcome.
This patch is intended to allow the rproc driver to handle the
following use-after-free issue:
### UAF log
[ 337.540275][ T470] virtqueue_add.llvm.10153462284424984632+0xb48/0
xcc4
[ 337.546969][ T470] virtqueue_add_inbuf+0x44/0x6c
<--- vqs are freed in vring_del_virtqueue
[ 337.551755][ T470] rpmsg_recv_done+0x1fc/0x2c4 [virtio_rpmsg_bus]
[ 337.558023][ T470] vring_interrupt+0xa0/0xe0
[ 337.562462][ T470] rproc_vq_interrupt+0x34/0x48
[ 337.567160][ T470] handle_event+0x28/0x48 [mtk_pqu_rproc]
[ 337.572742][ T470] irq_thread_fn+0x4c/0xcc
[ 337.577009][ T470] irq_thread+0x1d0/0x360
[ 337.581189][ T470] kthread+0x168/0x1cc
[ 337.585107][ T470] ret_from_fork+0x10/0x20
### stack trace of obj free
[ 339.253063][ T470] die_helper: <-
vring_del_virtqueue+0x16c/0x198
[ 339.259757][ T470] die_helper: <- kfree+0x274/0x35c
[ 339.265236][ T470] die_helper: <-
vring_del_virtqueue+0x16c/0x198
[ 339.271929][ T470] die_helper: <-
rproc_virtio_del_vqs+0x3c/0x58
[ 339.278535][ T470] die_helper: <- rpmsg_remove+0x8c/0x12c
[virtio_rpmsg_bus]
[ 339.286189][ T470] die_helper: <-
virtio_dev_remove+0x64/0x174
[ 339.292622][ T470] die_helper: <-
device_release_driver_internal+0x1a8/0x32c
[ 339.300270][ T470] die_helper: <-
bus_remove_device+0x130/0x160
[ 339.306789][ T470] die_helper: <- device_del+0x224/0x518
[ 339.312702][ T470] die_helper: <- device_unregister+0x20/0x3c
[ 339.319047][ T470] die_helper: <-
rproc_remove_virtio_dev+0x20/0x44
[ 339.325913][ T470] die_helper: <-
device_for_each_child+0x84/0x100
[ 339.332696][ T470] die_helper: <-
rproc_vdev_do_stop+0x30/0x5c
[ 339.339130][ T470] die_helper: <- rproc_stop+0xcc/0x200
On Fri, 2024-09-20 at 17:16 +0800, Mark-PK Tsai wrote:
> Add synchornize_cbs to rproc_virtio_config_ops and a
> synchronize_vqs callback to the rproc_ops to ensure vqs'
> state changes are visible in vring_interrupts when the vq is
> broken or removed.
>
> And when VIRTIO_HARDEN_NOTIFICATION is not set, call
> rproc_virtio_synchronize_cbs directly in __rproc_virtio_del_vqs
> before virtqueue is free to ensure that rproc_vq_interrupt is
> aware of the virtqueue removal.
>
> The synchronized_vqs is expected to be implemented in rproc
> driver to ensure that all previous vring_interrupts are handled
> before the vqs are marked as broken or removed.
>
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@xxxxxxxxxxxx>
> ---
> drivers/remoteproc/remoteproc_virtio.c | 12 ++++++++++++
> include/linux/remoteproc.h | 4 ++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
> b/drivers/remoteproc/remoteproc_virtio.c
> index d3f39009b28e..e18258b69851 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -74,6 +74,14 @@ static bool rproc_virtio_notify(struct virtqueue
> *vq)
> return true;
> }
>
> +static void rproc_virtio_synchronize_cbs(struct virtio_device *vdev)
> +{
> + struct rproc *rproc = vdev_to_rproc(vdev);
> +
> + if (rproc->ops->synchronize_vqs)
> + rproc->ops->synchronize_vqs(rproc);
> +}
> +
> /**
> * rproc_vq_interrupt() - tell remoteproc that a virtqueue is
> interrupted
> * @rproc: handle to the remote processor
> @@ -171,6 +179,9 @@ static void __rproc_virtio_del_vqs(struct
> virtio_device *vdev)
> list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
> rvring = vq->priv;
> rvring->vq = NULL;
> +#ifndef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> + rproc_virtio_synchronize_cbs(vdev);
> +#endif
> vring_del_virtqueue(vq);
> }
> }
> @@ -334,6 +345,7 @@ static const struct virtio_config_ops
> rproc_virtio_config_ops = {
> .get_status = rproc_virtio_get_status,
> .get = rproc_virtio_get,
> .set = rproc_virtio_set,
> + .synchronize_cbs = rproc_virtio_synchronize_cbs,
> };
>
> /*
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index b4795698d8c2..73901678ac7d 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -381,6 +381,9 @@ enum rsc_handling_status {
> * @panic: optional callback to react to system panic, core will
> delay
> * panic at least the returned number of milliseconds
> * @coredump: collect firmware dump after the subsystem is
> shutdown
> + * @synchronize_vqs: optional callback to guarantee all memory
> operations
> + * on the virtqueue before it are visible to the
> + * rproc_vq_interrupt().
> */
> struct rproc_ops {
> int (*prepare)(struct rproc *rproc);
> @@ -403,6 +406,7 @@ struct rproc_ops {
> u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware
> *fw);
> unsigned long (*panic)(struct rproc *rproc);
> void (*coredump)(struct rproc *rproc);
> + void (*synchronize_vqs)(struct rproc *rproc);
> };
>
> /**
