Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 12:48:14PM +0900, SeokYeon Hwang wrote:
> > On Wed, 2014-06-25 at 15:03 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 25, 2014 at 06:59:08PM +0900, SeokYeon Hwang wrote:
> > > > In case of the unrealized "pdev", memory can be illegally accessed and
> > corrupted.
> > > > Refer to device_unparent() in the commit
> > 5c21ce77d7e5643089ceec556c0408445d017f32.
> > Hi,
> > Thank you for submitting the patch.
> > 
> > Can you please send to the list how to reproduce the issue?
> > Is this a regression? Before the above commit did it work?
> 
> Hi,
> 
> When VirtIO device is hot-unplugged, Qemu unrealizes the device first then 
> child_bus by device_unparent().
> (After the commit 5c21ce77d7e5643089ceec556c0408445d017f32.)
> 
> For example, if I suppose to hot-unplug "virtio block" device, unparent 
> "virtio-blk-pci" first, then "virtio-blk".
> 
> virtio_bus_device_unplugged() function in virtio-bus.c calls "msix_uninit()" 
> with the parameter of its parent PCI device.
> The problem is that it accesses already freed member of pdev, such as 
> "pdev->config".
> This illegal access causes the heap corruption, and it kills Qemu process 
> suddenly.
> 
> You can detect this illegal access by using "electric fence" or "valgrind".

Does this still trigger for you with latest master?
I tried this:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 17ed510..fc81803 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -791,6 +791,11 @@ static void pci_config_free(PCIDevice *pci_dev)
     g_free(pci_dev->wmask);
     g_free(pci_dev->w1cmask);
     g_free(pci_dev->used);
+    pci_dev->config = NULL;
+    pci_dev->cmask = NULL;
+    pci_dev->wmask = NULL;
+    pci_dev->w1cmask = NULL;
+    pci_dev->used = NULL;
 }
 
 static void do_pci_unregister_device(PCIDevice *pci_dev)


does not trigger crash after hot-unplug of virtio-net-pci on master.


> > 
> > > >
> > > > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae
> > > > Signed-off-by: SeokYeon Hwang <address@hidden>
> > >
> > > Another case of qemu mailing list dropping patches :( I will bounce it
> > > now so people can see the original message.
> > >
> > >
> > > > ---
> > > >  hw/pci/pci.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 49eca95..bb7f0c5
> > > > 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -2056,7 +2056,12 @@ int pci_add_capability(PCIDevice *pdev,
> > > > uint8_t cap_id,
> > > >  /* Unlink capability from the pci config space. */  void
> > > > pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)  {
> > > > -    uint8_t prev, offset = pci_find_capability_list(pdev, cap_id,
> > &prev);
> > > > +    uint8_t prev, offset;
> > > > +    /* Check whether the device is realized or not */
> > > > +    if (!pdev->qdev.realized) {
> > The 'qdev' field and 'realize' property are private, you should use one of
> > QOM's 'property_get' methods.
> > 
> > I am also concerned about adding the realize check here, it seems too
> > "deep" in implementation, seeing this checks everywhere doesn't seem a
> > good idea.
> > I think we should first understand the root cause and try making this
> > change in a higher level.
> > 
> > I hope I helped,
> > Marcel
> 
> Ok, I see. I think you're right.
> If needed, I will send an another patch using "object_property_get_bool()".
> 
> Thanks.
> 
> > 
> > 
> > > > +        return;
> > > > +    }
> > > > +    offset = pci_find_capability_list(pdev, cap_id, &prev);
> > > >      if (!offset)
> > > >          return;
> > > >      pdev->config[prev] = pdev->config[offset + PCI_CAP_LIST_NEXT];
> > > > --
> > > > 1.9.1
> > >
> > 
>