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

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

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?

> > 
> > Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae
> > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> 
> 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

 
> > +        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
>