| Message ID | 20240920091659.3933-1-mark-pk.tsai@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio | expand |
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); > }; > > /**
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); > > }; > > > > /** >
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); }; /**
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(+)