Patchwork Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

login
register
mail settings
Submitter Gerd Hoffmann
Date May 19, 2011, 9:13 a.m.
Message ID <1305796393-22786-2-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/96331/
State New
Headers show

Comments

Gerd Hoffmann - May 19, 2011, 9:13 a.m.
This patch makes qemu ignore unplug requests from the guest for pci
devices which are tagged as non-hotpluggable.  Trouble spot is the
piix4 chipset with the ISA bridge.  Requests to unplug that one will
make it go away together with all ISA bus devices, which are not
prepared to be unplugged and thus don't cleanup, leaving active
qemu timers behind in free'ed memory.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/acpi_piix4.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Markus Armbruster - May 19, 2011, 10 a.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> This patch makes qemu ignore unplug requests from the guest for pci
> devices which are tagged as non-hotpluggable.  Trouble spot is the
> piix4 chipset with the ISA bridge.  Requests to unplug that one will
> make it go away together with all ISA bus devices, which are not
> prepared to be unplugged and thus don't cleanup, leaving active
> qemu timers behind in free'ed memory.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/acpi_piix4.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 96f5222..6c908ff 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -471,11 +471,13 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>      BusState *bus = opaque;
>      DeviceState *qdev, *next;
>      PCIDevice *dev;
> +    PCIDeviceInfo *info;
>      int slot = ffs(val) - 1;
>  
>      QLIST_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
>          dev = DO_UPCAST(PCIDevice, qdev, qdev);
> -        if (PCI_SLOT(dev->devfn) == slot) {
> +        info = container_of(qdev->info, PCIDeviceInfo, qdev);
> +        if (PCI_SLOT(dev->devfn) == slot && !info->no_hotplug) {
>              qdev_free(qdev);
>          }
>      }

Looks good, but what about pcie_cap_slot_hotplug()?
Gerd Hoffmann - May 19, 2011, 11:12 a.m.
Hi,

>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 96f5222..6c908ff 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -471,11 +471,13 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>>       BusState *bus = opaque;
>>       DeviceState *qdev, *next;
>>       PCIDevice *dev;
>> +    PCIDeviceInfo *info;
>>       int slot = ffs(val) - 1;
>>
>>       QLIST_FOREACH_SAFE(qdev,&bus->children, sibling, next) {
>>           dev = DO_UPCAST(PCIDevice, qdev, qdev);
>> -        if (PCI_SLOT(dev->devfn) == slot) {
>> +        info = container_of(qdev->info, PCIDeviceInfo, qdev);
>> +        if (PCI_SLOT(dev->devfn) == slot&&  !info->no_hotplug) {
>>               qdev_free(qdev);
>>           }
>>       }
>
> Looks good, but what about pcie_cap_slot_hotplug()?

Dunno, didn't look at q35 yet.  I'd expect the root bus isn't 
hot-pluggable, so the guest wouldn't be able to rip out any essential 
chipset devices.  But having someone more familier with pcie + q35 
double-check would be good ...

cheers,
   Gerd
Markus Armbruster - May 19, 2011, 11:23 a.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>>
>>> This patch makes qemu ignore unplug requests from the guest for pci
>>> devices which are tagged as non-hotpluggable.  Trouble spot is the
>>> piix4 chipset with the ISA bridge.  Requests to unplug that one will
>>> make it go away together with all ISA bus devices, which are not
>>> prepared to be unplugged and thus don't cleanup, leaving active
>>> qemu timers behind in free'ed memory.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  hw/acpi_piix4.c |    4 +++-
>>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>>> index 96f5222..6c908ff 100644
>>> --- a/hw/acpi_piix4.c
>>> +++ b/hw/acpi_piix4.c
>>> @@ -471,11 +471,13 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>>>       BusState *bus = opaque;
>>>       DeviceState *qdev, *next;
>>>       PCIDevice *dev;
>>> +    PCIDeviceInfo *info;
>>>       int slot = ffs(val) - 1;
>>>
>>>       QLIST_FOREACH_SAFE(qdev,&bus->children, sibling, next) {
>>>           dev = DO_UPCAST(PCIDevice, qdev, qdev);
>>> -        if (PCI_SLOT(dev->devfn) == slot) {
>>> +        info = container_of(qdev->info, PCIDeviceInfo, qdev);
>>> +        if (PCI_SLOT(dev->devfn) == slot&&  !info->no_hotplug) {
>>>               qdev_free(qdev);
>>>           }
>>>       }
>>
>> Looks good, but what about pcie_cap_slot_hotplug()?
>
> Dunno, didn't look at q35 yet.  I'd expect the root bus isn't
> hot-pluggable, so the guest wouldn't be able to rip out any essential
> chipset devices.  But having someone more familier with pcie + q35
> double-check would be good ...

I guess that would be Isaku Yamahata (cc'ed).
Isaku Yamahata - May 19, 2011, 11:52 a.m.
On Thu, May 19, 2011 at 01:23:18PM +0200, Markus Armbruster wrote:
> Gerd Hoffmann <kraxel@redhat.com> writes:
> 
> >   Hi,
> >
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> Gerd Hoffmann <kraxel@redhat.com> writes:
> >>
> >>> This patch makes qemu ignore unplug requests from the guest for pci
> >>> devices which are tagged as non-hotpluggable.  Trouble spot is the
> >>> piix4 chipset with the ISA bridge.  Requests to unplug that one will
> >>> make it go away together with all ISA bus devices, which are not
> >>> prepared to be unplugged and thus don't cleanup, leaving active
> >>> qemu timers behind in free'ed memory.
> >>>
> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>> ---
> >>>  hw/acpi_piix4.c |    4 +++-
> >>>  1 files changed, 3 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> >>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> >>> index 96f5222..6c908ff 100644
> >>> --- a/hw/acpi_piix4.c
> >>> +++ b/hw/acpi_piix4.c
> >>> @@ -471,11 +471,13 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
> >>>       BusState *bus = opaque;
> >>>       DeviceState *qdev, *next;
> >>>       PCIDevice *dev;
> >>> +    PCIDeviceInfo *info;
> >>>       int slot = ffs(val) - 1;
> >>>
> >>>       QLIST_FOREACH_SAFE(qdev,&bus->children, sibling, next) {
> >>>           dev = DO_UPCAST(PCIDevice, qdev, qdev);
> >>> -        if (PCI_SLOT(dev->devfn) == slot) {
> >>> +        info = container_of(qdev->info, PCIDeviceInfo, qdev);
> >>> +        if (PCI_SLOT(dev->devfn) == slot&&  !info->no_hotplug) {
> >>>               qdev_free(qdev);
> >>>           }
> >>>       }
> >>
> >> Looks good, but what about pcie_cap_slot_hotplug()?
> >
> > Dunno, didn't look at q35 yet.  I'd expect the root bus isn't
> > hot-pluggable, so the guest wouldn't be able to rip out any essential
> > chipset devices.  But having someone more familier with pcie + q35
> > double-check would be good ...
> 
> I guess that would be Isaku Yamahata (cc'ed).

The root pci bus of q35 isn't hot pluggable. The pcie bus with
the hotplug capability means that the slot in the bus is always
hot pluggable. So pcie_cap_slot_hotplug() doesn't need to check
no_hotplug.

If some sort of check is wanted, the check should be done at
the device initialization, I think.
Populating a non-hotpluggable devince in hot pluggable slot doesn't make
sense.

thanks,

Patch

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 96f5222..6c908ff 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -471,11 +471,13 @@  static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
     BusState *bus = opaque;
     DeviceState *qdev, *next;
     PCIDevice *dev;
+    PCIDeviceInfo *info;
     int slot = ffs(val) - 1;
 
     QLIST_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
         dev = DO_UPCAST(PCIDevice, qdev, qdev);
-        if (PCI_SLOT(dev->devfn) == slot) {
+        info = container_of(qdev->info, PCIDeviceInfo, qdev);
+        if (PCI_SLOT(dev->devfn) == slot && !info->no_hotplug) {
             qdev_free(qdev);
         }
     }