[PATCH 0/2] qemu: hotplug: Fix use-after-free on media change via 'virsh attach-device'
Reported via gitlab: https://gitlab.com/libvirt/libvirt/-/issues/859 I'll also later explore if it's possible to refactor the code to not touch the device definition wrappers after hotplug so that the pointer can be cleared properly when it's stolen into the domain definition. Peter Krempa (2): qemuDomainAttachDeviceDiskLive: Remove 'disk' variable qemu: hotplug: Don't access disk definititon after it was freed after media change src/qemu/qemu_hotplug.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Remove the extra temporary variable to make the changes in the next patch more obvious. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b3f2a173a8..6235f6c27c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1134,20 +1134,19 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriver *driver, virDomainObj *vm, virDomainDeviceDef *dev) { - virDomainDiskDef *disk = dev->data.disk; virDomainDiskDef *orig_disk = NULL; /* this API overloads media change semantics on disk hotplug * for devices supporting media changes */ - if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || - disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && - (orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { + if ((dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && + (orig_disk = virDomainDiskByTarget(vm->def, dev->data.disk->dst))) { if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, - disk->src, false) < 0) + dev->data.disk->src, false) < 0) return -1; - disk->src = NULL; - virDomainDiskDefFree(disk); + dev->data.disk->src = NULL; + virDomainDiskDefFree(dev->data.disk); return 0; } -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> A special case in qemuDomainAttachDeviceDiskLive causes disk media to be changed. This code has different semantics than the real hotplug code where the hotplugged device definition is absorbed into the domain definition and thus the pointer is still valid. On media change we just use the disk source and discard everything else from the disk definition. Later in qemuDomainAttachDeviceLive we then attempt to extract the alias of the attached device for emiting an event. Since in case of media change the main definition was freed this causes an use-after-free on the disk data pointer. To address this the media change code will clear the disk definition pointer from the device wrapper and the caller will extract the device alias only when the disk definition pointer is non-NULL. The semantics of the event will not change because the device alias wouldn't be assigned for the media change code at all. The use-after-free is observable via valgrind when attempting a media change via 'virsh attach-device', as otherwise in most cases it doesn't cause any ill efect as only the pointer to a NULL string is accessed: ==2763495== Invalid read of size 8 ==2763495== at 0xEA4102A: qemuDomainAttachDeviceLive (qemu_hotplug.c:3455) ==2763495== by 0xEA28ECD: qemuDomainAttachDeviceLiveAndConfig (qemu_driver.c:7408) ==2763495== by 0xEA28ECD: qemuDomainAttachDeviceFlags (qemu_driver.c:7456) ==2763495== by 0x4BC5BE6: virDomainAttachDevice (libvirt-domain.c:8951) ==2763495== by 0x402579D: remoteDispatchDomainAttachDevice (remote_daemon_dispatch_stubs.h:3763) [snip] ==2763495== Address 0x6df57c8 is 360 bytes inside a block of size 608 free'd ==2763495== at 0x48F7E43: free (vg_replace_malloc.c:990) ==2763495== by 0x4EC7EC4: g_free (in /usr/lib64/libglib-2.0.so.0.8600.3) ==2763495== by 0xEA4101E: qemuDomainAttachDeviceDiskLive (qemu_hotplug.c:1150) ==2763495== by 0xEA4101E: qemuDomainAttachDeviceLive (qemu_hotplug.c:3453) ==2763495== by 0xEA28ECD: qemuDomainAttachDeviceLiveAndConfig (qemu_driver.c:7408) ==2763495== by 0xEA28ECD: qemuDomainAttachDeviceFlags (qemu_driver.c:7456) ==2763495== by 0x4BC5BE6: virDomainAttachDevice (libvirt-domain.c:8951) [snip] Closes: https://gitlab.com/libvirt/libvirt/-/issues/859 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_hotplug.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6235f6c27c..df4bb49af7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1146,7 +1146,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriver *driver, return -1; dev->data.disk->src = NULL; - virDomainDiskDefFree(dev->data.disk); + g_clear_pointer(&dev->data.disk, virDomainDiskDefFree); return 0; } @@ -3450,7 +3450,7 @@ qemuDomainAttachDeviceLive(virDomainObj *vm, case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, NULL); ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); - if (!ret) { + if (ret == 0 && dev->data.disk) { alias = dev->data.disk->info.alias; dev->data.disk = NULL; } -- 2.53.0
On 3/6/26 08:16, Peter Krempa via Devel wrote:
Reported via gitlab:
https://gitlab.com/libvirt/libvirt/-/issues/859
I'll also later explore if it's possible to refactor the code to not touch the device definition wrappers after hotplug so that the pointer can be cleared properly when it's stolen into the domain definition.
Peter Krempa (2): qemuDomainAttachDeviceDiskLive: Remove 'disk' variable qemu: hotplug: Don't access disk definititon after it was freed after media change
src/qemu/qemu_hotplug.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa