* [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio
@ 2024-09-20 9:16 Mark-PK Tsai
2024-09-27 2:51 ` Mark-PK Tsai (蔡沛剛)
0 siblings, 1 reply; 3+ messages in thread
From: Mark-PK Tsai @ 2024-09-20 9:16 UTC (permalink / raw)
To: Bjorn Andersson, Mathieu Poirier, Matthias Brugger,
AngeloGioacchino Del Regno
Cc: yj.chiang, jy.ho, Mark-PK Tsai, linux-remoteproc, linux-kernel,
linux-arm-kernel, linux-mediatek
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio
2024-09-20 9:16 [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio Mark-PK Tsai
@ 2024-09-27 2:51 ` Mark-PK Tsai (蔡沛剛)
2024-10-02 15:36 ` Mathieu Poirier
0 siblings, 1 reply; 3+ messages in thread
From: Mark-PK Tsai (蔡沛剛) @ 2024-09-27 2:51 UTC (permalink / raw)
To: mathieu.poirier@linaro.org, matthias.bgg@gmail.com,
andersson@kernel.org, AngeloGioacchino Del Regno
Cc: linux-remoteproc@vger.kernel.org,
JY Ho (何駿彥), linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
YJ Chiang (江英杰)
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);
> };
>
> /**
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio
2024-09-27 2:51 ` Mark-PK Tsai (蔡沛剛)
@ 2024-10-02 15:36 ` Mathieu Poirier
0 siblings, 0 replies; 3+ messages in thread
From: Mathieu Poirier @ 2024-10-02 15:36 UTC (permalink / raw)
To: Mark-PK Tsai (蔡沛剛)
Cc: matthias.bgg@gmail.com, andersson@kernel.org,
AngeloGioacchino Del Regno, linux-remoteproc@vger.kernel.org,
JY Ho (何駿彥), linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
YJ Chiang (江英杰)
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);
> > };
> >
> > /**
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-02 15:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-20 9:16 [PATCH] remoteproc: virtio: Add synchronize_cbs to remoteproc_virtio Mark-PK Tsai
2024-09-27 2:51 ` Mark-PK Tsai (蔡沛剛)
2024-10-02 15:36 ` Mathieu Poirier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).