* [PATCH] block: Avoid stale pointer dereference in blk_get_aio_context()
@ 2020-07-09 13:50 Greg Kurz
2020-07-10 8:53 ` Kevin Wolf
0 siblings, 1 reply; 2+ messages in thread
From: Greg Kurz @ 2020-07-09 13:50 UTC (permalink / raw)
To: Kevin Wolf, Max Reitz; +Cc: qemu-devel, qemu-block
It is possible for blk_remove_bs() to race with blk_drain_all(), causing
the latter to dereference a stale blk->root pointer:
blk_remove_bs(blk)
bdrv_root_unref_child(blk->root)
child_bs = blk->root->bs
bdrv_detach_child(blk->root)
...
g_free(blk->root) <============== blk->root becomes stale
bdrv_unref(child_bs) <============ yield at some point
A blk_drain_all() can be triggered by some guest action in the
meantime, eg. on POWER, SLOF might disable bus mastering on
a virtio-scsi-pci device:
virtio_write_config()
virtio_pci_stop_ioeventfd()
virtio_bus_stop_ioeventfd()
virtio_scsi_dataplane_stop()
blk_drain_all()
blk_get_aio_context()
bs = blk->root ? blk->root->bs : NULL
^^^^^^^^^
stale
Then, depending on one's luck, QEMU either crashes with SEGV or
hits the assertion in blk_get_aio_context().
blk->root is set by blk_insert_bs() which calls bdrv_root_attach_child()
first. The blk_remove_bs() function should rollback the changes made
by blk_insert_bs() in the opposite order (or it should be documented
somewhere why this isn't the case). Clear blk->root before calling
bdrv_root_unref_child() in blk_remove_bs().
Signed-off-by: Greg Kurz <groug@kaod.org>
---
block/block-backend.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 6936b25c836c..0bf0188133e3 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -808,6 +808,7 @@ void blk_remove_bs(BlockBackend *blk)
{
ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
BlockDriverState *bs;
+ BdrvChild *root;
notifier_list_notify(&blk->remove_bs_notifiers, blk);
if (tgm->throttle_state) {
@@ -825,8 +826,9 @@ void blk_remove_bs(BlockBackend *blk)
* to avoid that and a potential QEMU crash.
*/
blk_drain(blk);
- bdrv_root_unref_child(blk->root);
+ root = blk->root;
blk->root = NULL;
+ bdrv_root_unref_child(root);
}
/*
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] block: Avoid stale pointer dereference in blk_get_aio_context()
2020-07-09 13:50 [PATCH] block: Avoid stale pointer dereference in blk_get_aio_context() Greg Kurz
@ 2020-07-10 8:53 ` Kevin Wolf
0 siblings, 0 replies; 2+ messages in thread
From: Kevin Wolf @ 2020-07-10 8:53 UTC (permalink / raw)
To: Greg Kurz; +Cc: qemu-devel, qemu-block, Max Reitz
Am 09.07.2020 um 15:50 hat Greg Kurz geschrieben:
> It is possible for blk_remove_bs() to race with blk_drain_all(), causing
> the latter to dereference a stale blk->root pointer:
>
>
> blk_remove_bs(blk)
> bdrv_root_unref_child(blk->root)
> child_bs = blk->root->bs
> bdrv_detach_child(blk->root)
> ...
> g_free(blk->root) <============== blk->root becomes stale
> bdrv_unref(child_bs) <============ yield at some point
>
> A blk_drain_all() can be triggered by some guest action in the
> meantime, eg. on POWER, SLOF might disable bus mastering on
> a virtio-scsi-pci device:
>
> virtio_write_config()
> virtio_pci_stop_ioeventfd()
> virtio_bus_stop_ioeventfd()
> virtio_scsi_dataplane_stop()
> blk_drain_all()
> blk_get_aio_context()
> bs = blk->root ? blk->root->bs : NULL
> ^^^^^^^^^
> stale
>
> Then, depending on one's luck, QEMU either crashes with SEGV or
> hits the assertion in blk_get_aio_context().
>
> blk->root is set by blk_insert_bs() which calls bdrv_root_attach_child()
> first. The blk_remove_bs() function should rollback the changes made
> by blk_insert_bs() in the opposite order (or it should be documented
> somewhere why this isn't the case). Clear blk->root before calling
> bdrv_root_unref_child() in blk_remove_bs().
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-10 8:54 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 13:50 [PATCH] block: Avoid stale pointer dereference in blk_get_aio_context() Greg Kurz
2020-07-10 8:53 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.