* [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
@ 2014-06-25 9:59 SeokYeon Hwang
2014-06-25 12:03 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: SeokYeon Hwang @ 2014-06-25 9:59 UTC (permalink / raw)
To: qemu-devel, mst; +Cc: syeon.hwang
In case of the unrealized "pdev", memory can be illegally accessed and corrupted.
Refer to device_unparent() in the commit 5c21ce77d7e5643089ceec556c0408445d017f32.
Change-Id: Iacb195a092c86d4c677ad0404582af104b2251ae
Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
---
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) {
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
2014-06-25 9:59 [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability SeokYeon Hwang
@ 2014-06-25 12:03 ` Michael S. Tsirkin
2014-06-25 12:28 ` Marcel Apfelbaum
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-06-25 12:03 UTC (permalink / raw)
To: SeokYeon Hwang; +Cc: qemu-devel
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.
>
> 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) {
> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
2014-06-25 12:03 ` Michael S. Tsirkin
@ 2014-06-25 12:28 ` Marcel Apfelbaum
2014-06-26 3:48 ` SeokYeon Hwang
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Apfelbaum @ 2014-06-25 12:28 UTC (permalink / raw)
To: SeokYeon Hwang; +Cc: qemu-devel@nongnu.org, Michael S. Tsirkin
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
2014-06-25 12:28 ` Marcel Apfelbaum
@ 2014-06-26 3:48 ` SeokYeon Hwang
2014-06-26 7:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 6+ messages in thread
From: SeokYeon Hwang @ 2014-06-26 3:48 UTC (permalink / raw)
To: marcel.a; +Cc: qemu-devel, 'Michael S. Tsirkin'
> 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".
>
> > >
> > > 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
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
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
2014-06-26 3:48 ` SeokYeon Hwang
@ 2014-06-26 7:07 ` Michael S. Tsirkin
2014-06-28 6:39 ` SeokYeon Hwang
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2014-06-26 7:07 UTC (permalink / raw)
To: SeokYeon Hwang; +Cc: qemu-devel, marcel.a
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 <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
>
> 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
> > >
> >
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability
2014-06-26 7:07 ` Michael S. Tsirkin
@ 2014-06-28 6:39 ` SeokYeon Hwang
0 siblings, 0 replies; 6+ messages in thread
From: SeokYeon Hwang @ 2014-06-28 6:39 UTC (permalink / raw)
To: 'Michael S. Tsirkin'; +Cc: syeon.hwang, qemu-devel, marcel.a
When I check it the latest master branch, the issue is already resolved.
It is resolved by commit 5942a19040fed313b316ab7b6e3d2d8e7b1625bb (28,
June).
I'm sorry and thank you for your help.
> 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 <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
> >
> > 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
> > > >
> > >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-28 6:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-25 9:59 [Qemu-devel] [PATCH] pci: add checking whether the device is realized before unlinking the capability SeokYeon Hwang
2014-06-25 12:03 ` Michael S. Tsirkin
2014-06-25 12:28 ` Marcel Apfelbaum
2014-06-26 3:48 ` SeokYeon Hwang
2014-06-26 7:07 ` Michael S. Tsirkin
2014-06-28 6:39 ` SeokYeon Hwang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).