remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio

[PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio

Posted by Mark-PK Tsai 2 months, 1 week ago

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@mediatek.com>
---
 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);
 };
 
 /**
-- 
2.45.2

Re: [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio

Posted by Mark-PK Tsai (蔡沛剛) 2 months ago

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@mediatek.com>
> ---
>  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);
>  };
>  
>  /**

Re: [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio

Posted by Mathieu Poirier 1 month, 3 weeks ago

On Fri, Sep 27, 2024 at 02:51:38AM +0000, Mark-PK Tsai (蔡沛剛) wrote:
> 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

What does "UAF" means?

> [  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
> 

None of the above is showing me where this use-after-free happens.


> 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@mediatek.com>
> > ---
> >  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,

Where and when is this called?

This patch is very confusing and doesn't have a user.  As such it is
impossible for me to understand what is going on.  

Thanks,
Mathieu

> >  };
> >  
> >  /*
> > 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);
> >  };
> >  
> >  /**
>