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 <address@hidden> writes:
> 
> >   Hi,
> >
> > Markus Armbruster <address@hidden> writes:
> >
> >> Gerd Hoffmann <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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