| Message ID | 20221121211923.1993171-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-7.2] block-backend: avoid bdrv_unregister_buf() NULL pointer deref | expand |
On 21/11/22 22:19, Stefan Hajnoczi wrote: > bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL > leads to undefined behavior. > > Jonathan Cameron reported this following NULL pointer dereference when a > VM with a virtio-blk device and a memory-backend-file object is > terminated: > 1. qemu_cleanup() closes all drives, setting blk->root to NULL > 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM > block notifier callback because the memory-backend-file is destroyed. > 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar > notifier callback and undefined behavior occurs. > > Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") > Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/block-backend.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 21.11.2022 um 22:19 hat Stefan Hajnoczi geschrieben: > bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL > leads to undefined behavior. > > Jonathan Cameron reported this following NULL pointer dereference when a > VM with a virtio-blk device and a memory-backend-file object is > terminated: > 1. qemu_cleanup() closes all drives, setting blk->root to NULL > 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM > block notifier callback because the memory-backend-file is destroyed. > 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar > notifier callback and undefined behavior occurs. > > Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") > Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> This raises some questions, though. What happens if the graph isn't static between creation and deletion of the device? Do we need to do something with registered buffers when a node is attached to or detached from an existing device? Kevin
On Tue, 22 Nov 2022 at 03:22, Kevin Wolf <kwolf@redhat.com> wrote: > > Am 21.11.2022 um 22:19 hat Stefan Hajnoczi geschrieben: > > bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL > > leads to undefined behavior. > > > > Jonathan Cameron reported this following NULL pointer dereference when a > > VM with a virtio-blk device and a memory-backend-file object is > > terminated: > > 1. qemu_cleanup() closes all drives, setting blk->root to NULL > > 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM > > block notifier callback because the memory-backend-file is destroyed. > > 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar > > notifier callback and undefined behavior occurs. > > > > Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") > > Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > > This raises some questions, though. What happens if the graph isn't > static between creation and deletion of the device? Do we need to do > something with registered buffers when a node is attached to or detached > from an existing device? I think you are right. Graph changes need to be handled. Right now they aren't. Stefan
Merged. I will work on supporting graph changes. Stefan
diff --git a/block/block-backend.c b/block/block-backend.c index b48c91f4e1..d98a96ff37 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2576,14 +2576,25 @@ static void blk_root_drained_end(BdrvChild *child, int *drained_end_counter) bool blk_register_buf(BlockBackend *blk, void *host, size_t size, Error **errp) { + BlockDriverState *bs = blk_bs(blk); + GLOBAL_STATE_CODE(); - return bdrv_register_buf(blk_bs(blk), host, size, errp); + + if (bs) { + return bdrv_register_buf(bs, host, size, errp); + } + return true; } void blk_unregister_buf(BlockBackend *blk, void *host, size_t size) { + BlockDriverState *bs = blk_bs(blk); + GLOBAL_STATE_CODE(); - bdrv_unregister_buf(blk_bs(blk), host, size); + + if (bs) { + bdrv_unregister_buf(bs, host, size); + } } int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
bdrv_*() APIs expect a valid BlockDriverState. Calling them with bs=NULL leads to undefined behavior. Jonathan Cameron reported this following NULL pointer dereference when a VM with a virtio-blk device and a memory-backend-file object is terminated: 1. qemu_cleanup() closes all drives, setting blk->root to NULL 2. qemu_cleanup() calls user_creatable_cleanup(), which results in a RAM block notifier callback because the memory-backend-file is destroyed. 3. blk_unregister_buf() is called by virtio-blk's BlockRamRegistrar notifier callback and undefined behavior occurs. Fixes: baf422684d73 ("virtio-blk: use BDRV_REQ_REGISTERED_BUF optimization hint") Co-authored-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/block-backend.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)