[Qemu-devel] [PULL] Fix CVE-2011-1751.

[Qemu-devel] [PULL] Fix CVE-2011-1751.

[Gerd Hoffmann]

  Hi folks,

Urgent security fix.  Details are in the commit message.

please pull,
  Gerd

Gerd Hoffmann (1):
  Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

 hw/acpi_piix4.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

The following changes since commit 96d19bcbf5f679bbaaeab001b572c367fbfb2b03:

  ahci: Unbreak bar registration (2011-05-16 10:15:47 -0500)

are available in the git repository at:
  git://git.kraxel.org/qemu CVE-2011-1751

Gerd Hoffmann (1):
      Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

 hw/acpi_piix4.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

[Qemu-devel] [PATCH] Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

[Gerd Hoffmann]

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);
         }
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH] Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

[Markus Armbruster]

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()?

Re: [Qemu-devel] [PATCH] Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

[Gerd Hoffmann]

   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

Re: [Qemu-devel] [PATCH] Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

[Markus Armbruster]

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).

Re: [Qemu-devel] [PATCH] Ignore pci unplug requests for unpluggable devices (CVE-2011-1751)

[Isaku Yamahata]

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,
-- 
yamahata