* [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
@ 2017-08-22 21:43 Philippe Mathieu-Daudé
2017-08-22 22:42 ` Michael S. Tsirkin
2017-08-23 8:32 ` Igor Mammedov
0 siblings, 2 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-08-22 21:43 UTC (permalink / raw)
To: Igor Mammedov, Michael S . Tsirkin, Thomas Huth, John Snow
Cc: Philippe Mathieu-Daudé, qemu-devel, Qemu-block
9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
'use_acpi_pci_hotplug' for pc-1.7 and older machines.
c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
property.
Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
The following valgrind Trace equivs:
qdev_device_add( "ich9-ahci" )
-> device_set_realized()
-> hotplug_handler_plug()
-> piix4_device_plug_cb()
-> acpi_pcihp_device_plug_cb()
-> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
-> object_unparent()
<- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
$ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
(qemu) device_add ich9-ahci,id=ich9-ahci
==6604== Invalid read of size 8
==6604== at 0x609AB0: object_unparent (object.c:445)
==6604== by 0x4C4478: device_unparent (qdev.c:1095)
==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
==6604== by 0x364311: monitor_read (monitor.c:3905)
==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
==6604== by 0x608DCD: property_set_bool (object.c:1886)
==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
==6604== Block was alloc'd at
==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==6604== by 0x50094F: ahci_realize (ahci.c:1468)
==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
==6604== by 0x608DCD: property_set_bool (object.c:1886)
==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
Reported-by: Thomas Huth <thuth@redhat.com>
Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/acpi/piix4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f276967365..d4df209a2e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
dev, errp);
}
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
- if (!xen_enabled()) {
+ if (s->use_acpi_pci_hotplug) {
acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
errp);
}
@@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
dev, errp);
} else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
- if (!xen_enabled()) {
+ if (s->use_acpi_pci_hotplug) {
acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
errp);
}
--
2.14.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-22 21:43 [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat Philippe Mathieu-Daudé
@ 2017-08-22 22:42 ` Michael S. Tsirkin
2017-08-23 0:10 ` Philippe Mathieu-Daudé
2017-08-23 8:32 ` Igor Mammedov
1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-08-22 22:42 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Igor Mammedov, Thomas Huth, John Snow, qemu-devel, Qemu-block
On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
>
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>
> The following valgrind Trace equivs:
>
> qdev_device_add( "ich9-ahci" )
> -> device_set_realized()
> -> hotplug_handler_plug()
> -> piix4_device_plug_cb()
> -> acpi_pcihp_device_plug_cb()
> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
> -> object_unparent()
> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604== at 0x609AB0: object_unparent (object.c:445)
> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604== by 0x364311: monitor_read (monitor.c:3905)
> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== Block was alloc'd at
> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Looks like this is a very old bug, isn't it?
Objections to merging this after the release?
> ---
> hw/acpi/piix4.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> dev, errp);
> }
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
> dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
> --
> 2.14.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-22 22:42 ` Michael S. Tsirkin
@ 2017-08-23 0:10 ` Philippe Mathieu-Daudé
2017-08-23 5:40 ` Thomas Huth
0 siblings, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-08-23 0:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Igor Mammedov, Thomas Huth, John Snow, qemu-devel, Qemu-block
On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>> property.
>>
>> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>>
>> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>>
>> The following valgrind Trace equivs:
>>
>> qdev_device_add( "ich9-ahci" )
>> -> device_set_realized()
>> -> hotplug_handler_plug()
>> -> piix4_device_plug_cb()
>> -> acpi_pcihp_device_plug_cb()
>> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
>> -> object_unparent()
>> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>>
>> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
>> (qemu) device_add ich9-ahci,id=ich9-ahci
>> ==6604== Invalid read of size 8
>> ==6604== at 0x609AB0: object_unparent (object.c:445)
>> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
>> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
>> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
>> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
>> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
>> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
>> ==6604== by 0x364311: monitor_read (monitor.c:3905)
>> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
>> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
>> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
>> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
>> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
>> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
>> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
>> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
>> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
>> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
>> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
>> ==6604== Block was alloc'd at
>> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
>> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
>> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
>> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
>> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
>> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
>> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
>> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
>> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
>> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
>> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
>> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>>
>> Reported-by: Thomas Huth <thuth@redhat.com>
>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Looks like this is a very old bug, isn't it?
> Objections to merging this after the release?
Yes, I'm also inclined to delay it so we can release 2.10, I tagged
"2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
let him decide if it is worth crying wolf :) It's very likely no-one but
him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
plugging AHCI devices :D
>
>> ---
>> hw/acpi/piix4.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index f276967365..d4df209a2e 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>> dev, errp);
>> }
>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> - if (!xen_enabled()) {
>> + if (s->use_acpi_pci_hotplug) {
>> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
>> errp);
>> }
>> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
>> dev, errp);
>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> - if (!xen_enabled()) {
>> + if (s->use_acpi_pci_hotplug) {
>> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
>> errp);
>> }
>> --
>> 2.14.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-23 0:10 ` Philippe Mathieu-Daudé
@ 2017-08-23 5:40 ` Thomas Huth
2017-08-23 6:04 ` Thomas Huth
2017-08-23 13:17 ` Michael S. Tsirkin
0 siblings, 2 replies; 8+ messages in thread
From: Thomas Huth @ 2017-08-23 5:40 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin
Cc: Igor Mammedov, John Snow, qemu-devel, qemu-block, qemu-stable
On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>>> property
>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>>> added the
>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>>> property.
>>>
>>> Check for use_acpi_pci_hotplug before calling
>>> acpi_pcihp_device_[un]plug_cb().
[...]
>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Looks like this is a very old bug, isn't it?
>> Objections to merging this after the release?
>
> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> let him decide if it is worth crying wolf :) It's very likely no-one but
> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> plugging AHCI devices :D
I'm fine if this gets included in 2.11 - it's quite unlikely that a user
tries hot-plug ahci on such an old machine type, I think. But we maybe
should include this in the 2.10.1 stable release, so I'm putting
qemu-stable on CC now.
Anyway, your patch seems to fix the issue for me, thanks!
Tested-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-23 5:40 ` Thomas Huth
@ 2017-08-23 6:04 ` Thomas Huth
2017-08-23 8:35 ` Igor Mammedov
2017-08-23 13:17 ` Michael S. Tsirkin
1 sibling, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2017-08-23 6:04 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin
Cc: Igor Mammedov, John Snow, qemu-devel, qemu-block, qemu-stable,
Aurelien Jarno
On 23.08.2017 07:40, Thomas Huth wrote:
> On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
>> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
>>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
>>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>>>> property
>>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>>>> added the
>>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>>>> property.
>>>>
>>>> Check for use_acpi_pci_hotplug before calling
>>>> acpi_pcihp_device_[un]plug_cb().
> [...]
>>>> Reported-by: Thomas Huth <thuth@redhat.com>
>>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> Looks like this is a very old bug, isn't it?
>>> Objections to merging this after the release?
>>
>> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
>> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
>> let him decide if it is worth crying wolf :) It's very likely no-one but
>> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
>> plugging AHCI devices :D
>
> I'm fine if this gets included in 2.11 - it's quite unlikely that a user
> tries hot-plug ahci on such an old machine type, I think. But we maybe
> should include this in the 2.10.1 stable release, so I'm putting
> qemu-stable on CC now.
>
> Anyway, your patch seems to fix the issue for me, thanks!
>
> Tested-by: Thomas Huth <thuth@redhat.com>
... No, I was too fast here. I'm afraid, it still crashes with mips64el:
$ valgrind mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest
==17935== Memcheck, a memory error detector
==17935== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==17935== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==17935== Command: mips64el-softmmu/qemu-system-mips64el -S -nographic -M malta,accel=qtest
==17935==
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add ich9-ahci
==17935== Invalid read of size 8
==17935== at 0x5F6F10: object_unparent (object.c:445)
==17935== by 0x4BB2C8: device_unparent (qdev.c:1095)
==17935== by 0x5F77C4: object_finalize_child_property (object.c:1396)
==17935== by 0x5F6706: object_property_del_child.isra.7 (object.c:427)
==17935== by 0x448BC8: qdev_device_add (qdev-monitor.c:634)
==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935== by 0x462B29: hmp_device_add (hmp.c:1925)
==17935== by 0x370F83: handle_hmp_command (monitor.c:3119)
==17935== by 0x371E59: monitor_command_cb (monitor.c:3922)
==17935== by 0x6D3187: readline_handle_byte (readline.c:393)
==17935== by 0x371211: monitor_read (monitor.c:3905)
==17935== by 0x6699D3: mux_chr_read (char-mux.c:216)
==17935== Address 0x21c549d8 is 30,328 bytes inside a block of size 36,288 free'd
==17935== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
==17935== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
==17935== by 0x4FB94E: pci_ich9_uninit (ich.c:161)
==17935== by 0x5350FB: pci_qdev_unrealize (pci.c:1083)
==17935== by 0x4BCD39: device_set_realized (qdev.c:988)
==17935== by 0x5F622D: property_set_bool (object.c:1886)
==17935== by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27)
==17935== by 0x5F7FCF: object_property_set_bool (object.c:1162)
==17935== by 0x448B93: qdev_device_add (qdev-monitor.c:630)
==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935== by 0x462B29: hmp_device_add (hmp.c:1925)
==17935== by 0x370F83: handle_hmp_command (monitor.c:3119)
==17935== Block was alloc'd at
==17935== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
==17935== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
==17935== by 0x4FB28F: ahci_realize (ahci.c:1468)
==17935== by 0x4FB9D8: pci_ich9_ahci_realize (ich.c:115)
==17935== by 0x5366BD: pci_qdev_realize (pci.c:2002)
==17935== by 0x4BCCB9: device_set_realized (qdev.c:914)
==17935== by 0x5F622D: property_set_bool (object.c:1886)
==17935== by 0x5FA31E: object_property_set_qobject (qom-qobject.c:27)
==17935== by 0x5F7FCF: object_property_set_bool (object.c:1162)
==17935== by 0x448B93: qdev_device_add (qdev-monitor.c:630)
==17935== by 0x449122: qmp_device_add (qdev-monitor.c:807)
==17935== by 0x462B29: hmp_device_add (hmp.c:1925)
Do you've got an idea how to fix that, too?
Thomas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-22 21:43 [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat Philippe Mathieu-Daudé
2017-08-22 22:42 ` Michael S. Tsirkin
@ 2017-08-23 8:32 ` Igor Mammedov
1 sibling, 0 replies; 8+ messages in thread
From: Igor Mammedov @ 2017-08-23 8:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Michael S . Tsirkin, Thomas Huth, John Snow, qemu-devel,
Qemu-block, Anthony PERARD
On Tue, 22 Aug 2017 18:43:43 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
>
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
>
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
>
> The following valgrind Trace equivs:
>
> qdev_device_add( "ich9-ahci" )
> -> device_set_realized()
> -> hotplug_handler_plug()
> -> piix4_device_plug_cb()
> -> acpi_pcihp_device_plug_cb()
> -> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
> -> object_unparent()
> <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
>
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604== at 0x609AB0: object_unparent (object.c:445)
> ==6604== by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604== by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604== by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604== by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604== by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604== by 0x364311: monitor_read (monitor.c:3905)
> ==6604== by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604== Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 free'd
> ==6604== at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604== by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604== by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604== by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604== by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604== Block was alloc'd at
> ==6604== at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604== by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604== by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604== by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604== by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604== by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604== by 0x608DCD: property_set_bool (object.c:1886)
> ==6604== by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604== by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604== by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604== by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604== by 0x46B689: hmp_device_add (hmp.c:1925)
>
> Reported-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/acpi/piix4.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> dev, errp);
> }
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
s->use_acpi_pci_hotplug is a switch between legacy and current modes
of PCI hotplug and acpi_pcihp_device_plug_cb() should be able to handle
both.
Looking at history train-wreck started since
(f0c9d64 pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice)
when bsel became solely allocated by another component (acpi table builder)
with follow up fixups to make that split brain (hw(legacy|modern)/acpi(in seabios|in qemu))
combos work somehow.
I think Anthony's (CCed) patches is the right way to fix issues not only
for Xen but also for legacy SeaBIOS as well.
[PATCH for-2.10 v3 0/3] Fix hotplug of PCI passthrought dev
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03216.html
Series still needs to be fixed (broken reboot logic)
but it might work for your case.
> @@ -411,7 +411,7 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
> dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> - if (!xen_enabled()) {
> + if (s->use_acpi_pci_hotplug) {
> acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> errp);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-23 6:04 ` Thomas Huth
@ 2017-08-23 8:35 ` Igor Mammedov
0 siblings, 0 replies; 8+ messages in thread
From: Igor Mammedov @ 2017-08-23 8:35 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
Michael S. Tsirkin, John Snow, qemu-devel, qemu-block,
qemu-stable, Aurelien Jarno
On Wed, 23 Aug 2017 08:04:06 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 23.08.2017 07:40, Thomas Huth wrote:
> > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
> >> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
> >>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
> >>>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
> >>>> property
> >>>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> >>>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
> >>>> added the
> >>>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> >>>> property.
> >>>>
> >>>> Check for use_acpi_pci_hotplug before calling
> >>>> acpi_pcihp_device_[un]plug_cb().
> > [...]
> >>>> Reported-by: Thomas Huth <thuth@redhat.com>
> >>>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>
> >>> Looks like this is a very old bug, isn't it?
> >>> Objections to merging this after the release?
> >>
> >> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> >> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> >> let him decide if it is worth crying wolf :) It's very likely no-one but
> >> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> >> plugging AHCI devices :D
> >
> > I'm fine if this gets included in 2.11 - it's quite unlikely that a user
> > tries hot-plug ahci on such an old machine type, I think. But we maybe
question is should be ahci device by hotpluggable at all?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat
2017-08-23 5:40 ` Thomas Huth
2017-08-23 6:04 ` Thomas Huth
@ 2017-08-23 13:17 ` Michael S. Tsirkin
1 sibling, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2017-08-23 13:17 UTC (permalink / raw)
To: Thomas Huth
Cc: Philippe Mathieu-Daudé,
Igor Mammedov, John Snow, qemu-devel, qemu-block, qemu-stable
On Wed, Aug 23, 2017 at 07:40:39AM +0200, Thomas Huth wrote:
> On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:
> > On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:
> >> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:
> >>> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
> >>> property
> >>> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> >>> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
> >>> added the
> >>> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> >>> property.
> >>>
> >>> Check for use_acpi_pci_hotplug before calling
> >>> acpi_pcihp_device_[un]plug_cb().
> [...]
> >>> Reported-by: Thomas Huth <thuth@redhat.com>
> >>> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b090@redhat.com>
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>
> >> Looks like this is a very old bug, isn't it?
> >> Objections to merging this after the release?
> >
> > Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> > "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> > let him decide if it is worth crying wolf :) It's very likely no-one but
> > him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> > plugging AHCI devices :D
>
> I'm fine if this gets included in 2.11 - it's quite unlikely that a user
> tries hot-plug ahci on such an old machine type, I think. But we maybe
> should include this in the 2.10.1 stable release, so I'm putting
> qemu-stable on CC now.
>
> Anyway, your patch seems to fix the issue for me, thanks!
>
> Tested-by: Thomas Huth <thuth@redhat.com>
ok, pls remember to repost or ping after the release.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-08-23 13:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-22 21:43 [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat Philippe Mathieu-Daudé
2017-08-22 22:42 ` Michael S. Tsirkin
2017-08-23 0:10 ` Philippe Mathieu-Daudé
2017-08-23 5:40 ` Thomas Huth
2017-08-23 6:04 ` Thomas Huth
2017-08-23 8:35 ` Igor Mammedov
2017-08-23 13:17 ` Michael S. Tsirkin
2017-08-23 8:32 ` Igor Mammedov
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.