[Qemu-devel] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Qemu-devel] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Li Qiang]

When getting the 'pbdev', the if...else has no default branch.
>From Coverity, the 'pbdev' maybe null when the 'dev' is not
the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
This patch adds a default branch for device plug and unplug.

Spotted by Coverity: CID 1398593

Signed-off-by: Li Qiang <liq3ea@163.com>
---
Adds a default branch for device plug per Cornelia's review.

 hw/s390x/s390-pci-bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 15759b6514..fe48a36ff6 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         pbdev->fh = pbdev->idx;
         QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
         g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
+    } else {
+        error_setg(errp, "s390: device plug request for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
     }
 }
 
@@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
         pbdev = S390_PCI_DEVICE(dev);
         pci_dev = pbdev->pdev;
+    } else {
+        error_setg(errp, "s390: device unplug request for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+        return;
     }
 
     switch (pbdev->state) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

On Thu,  3 Jan 2019 07:16:12 -0800
Li Qiang <liq3ea@163.com> wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
> Adds a default branch for device plug per Cornelia's review.
> 
>  hw/s390x/s390-pci-bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..fe48a36ff6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          pbdev->fh = pbdev->idx;
>          QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
>          g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
> +    } else {
> +        error_setg(errp, "s390: device plug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));

Maybe make this "s390/pci: plugging device type <%s> is not supported"?

>      }
>  }
>  
> @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>          pbdev = S390_PCI_DEVICE(dev);
>          pci_dev = pbdev->pdev;
> +    } else {
> +        error_setg(errp, "s390: device unplug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));

Halil has a point when he suggests an assert here. If we fence the
device type in the plug function, I can't think of a way we would end
up here.

> +        return;
>      }
>  
>      switch (pbdev->state) {

[Qemu-devel] 答复: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Li Qiang]

What do you think of ‘g_assert_not_reached();’. For example:

else {
     g_assert_not_reached();
}

Thanks,
Li Qiang


发件人: Cornelia Huck
发送时间: 2019年1月4日 22:10
收件人: Li Qiang
抄送: walling@linux.ibm.com; rth@twiddle.net; david@redhat.com; pasic@linux.ibm.com; borntraeger@de.ibm.com; qemu-s390x@nongnu.org; qemu-devel@nongnu.org; peter.maydell@linaro.org
主题: Re: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

On Thu,  3 Jan 2019 07:16:12 -0800
Li Qiang <liq3ea@163.com> wrote:

> When getting the 'pbdev', the if...else has no default branch.
> From Coverity, the 'pbdev' maybe null when the 'dev' is not
> the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> This patch adds a default branch for device plug and unplug.
> 
> Spotted by Coverity: CID 1398593
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
> Adds a default branch for device plug per Cornelia's review.
> 
>  hw/s390x/s390-pci-bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 15759b6514..fe48a36ff6 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          pbdev->fh = pbdev->idx;
>          QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
>          g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
> +    } else {
> +        error_setg(errp, "s390: device plug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));

Maybe make this "s390/pci: plugging device type <%s> is not supported"?

>      }
>  }
>  
> @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
>          pbdev = S390_PCI_DEVICE(dev);
>          pci_dev = pbdev->pdev;
> +    } else {
> +        error_setg(errp, "s390: device unplug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));

Halil has a point when he suggests an assert here. If we fence the
device type in the plug function, I can't think of a way we would end
up here.

> +        return;
>      }
>  
>      switch (pbdev->state) {

Re: [Qemu-devel] [qemu-s390x] 答复: [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[David Hildenbrand]

On 04.01.19 15:33, Li Qiang wrote:
> What do you think of ‘g_assert_not_reached();’. For example:
> 
>  
> 
> else {
> 
>      g_assert_not_reached();
> 
> }
> 

I agree, if thisever happens, it is a serious programming error, not an
error to report to the user. (after all, he did nothing wrong)

I'd prefer asserts.

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Fri, 4 Jan 2019 22:33:51 +0800
Li Qiang <liq3ea@163.com> wrote:

> What do you think of ‘g_assert_not_reached();’. For example:
> 
> else {
>      g_assert_not_reached();
> }

Sounds good. But please return anyway in the unplug case, so that the
code is fine if asserts have been configured out.

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Peter Maydell]

On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Fri, 4 Jan 2019 22:33:51 +0800
> Li Qiang <liq3ea@163.com> wrote:
>
> > What do you think of ‘g_assert_not_reached();’. For example:
> >
> > else {
> >      g_assert_not_reached();
> > }
>
> Sounds good. But please return anyway in the unplug case, so that the
> code is fine if asserts have been configured out.

Hopefully that won't cause the compiler to complain about
unreachable code :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Mon, 7 Jan 2019 15:54:21 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Fri, 4 Jan 2019 22:33:51 +0800
> > Li Qiang <liq3ea@163.com> wrote:
> >  
> > > What do you think of ‘g_assert_not_reached();’. For example:
> > >
> > > else {
> > >      g_assert_not_reached();
> > > }  
> >
> > Sounds good. But please return anyway in the unplug case, so that the
> > code is fine if asserts have been configured out.  
> 
> Hopefully that won't cause the compiler to complain about
> unreachable code :-)

BTW: Is there a common configuration where asserts are configured out?
Not that this is an accident waiting to happen...

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Peter Maydell]

On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote:
> On Mon, 7 Jan 2019 15:54:21 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote:
> > > Sounds good. But please return anyway in the unplug case, so that the
> > > code is fine if asserts have been configured out.
> >
> > Hopefully that won't cause the compiler to complain about
> > unreachable code :-)
>
> BTW: Is there a common configuration where asserts are configured out?
> Not that this is an accident waiting to happen...

No -- we insist they are always enabled, and osdep.h will #error
out if either NDEBUG or G_DISABLE_ASSERT are set.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[Cornelia Huck]

On Mon, 7 Jan 2019 16:04:35 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Mon, 7 Jan 2019 15:54:21 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote:  
> > > > Sounds good. But please return anyway in the unplug case, so that the
> > > > code is fine if asserts have been configured out.  
> > >
> > > Hopefully that won't cause the compiler to complain about
> > > unreachable code :-)  
> >
> > BTW: Is there a common configuration where asserts are configured out?
> > Not that this is an accident waiting to happen...  
> 
> No -- we insist they are always enabled, and osdep.h will #error
> out if either NDEBUG or G_DISABLE_ASSERT are set.

Ah, now I remember (I thought we still had that problem.)

In that case, no return is needed.

Re: [Qemu-devel] [PATCH v2] s390: avoid potential null dereference ins390_pcihost_unplug()

[李强]

At 2019-01-08 00:10:29, "Cornelia Huck" <cohuck@redhat.com> wrote:
>On Mon, 7 Jan 2019 16:04:35 +0000
>Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On Mon, 7 Jan 2019 at 15:57, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Mon, 7 Jan 2019 15:54:21 +0000
>> > Peter Maydell <peter.maydell@linaro.org> wrote:  
>> > > On Mon, 7 Jan 2019 at 15:48, Cornelia Huck <cohuck@redhat.com> wrote:  
>> > > > Sounds good. But please return anyway in the unplug case, so that the
>> > > > code is fine if asserts have been configured out.  
>> > >
>> > > Hopefully that won't cause the compiler to complain about
>> > > unreachable code :-)  
>> >
>> > BTW: Is there a common configuration where asserts are configured out?
>> > Not that this is an accident waiting to happen...  
>> 
>> No -- we insist they are always enabled, and osdep.h will #error
>> out if either NDEBUG or G_DISABLE_ASSERT are set.
>
>Ah, now I remember (I thought we still had that problem.)
>

>In that case, no return is needed.


Ok, later I will send out a revised patch.


Thanks,
Li Qiang

Re: [Qemu-devel] [qemu-s390x] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Halil Pasic]

On Fri, 4 Jan 2019 15:10:05 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu,  3 Jan 2019 07:16:12 -0800
> Li Qiang <liq3ea@163.com> wrote:
> 
> > When getting the 'pbdev', the if...else has no default branch.
> > From Coverity, the 'pbdev' maybe null when the 'dev' is not
> > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> > This patch adds a default branch for device plug and unplug.
> > 
> > Spotted by Coverity: CID 1398593
> > 
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> > Adds a default branch for device plug per Cornelia's review.
> > 
> >  hw/s390x/s390-pci-bus.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 15759b6514..fe48a36ff6 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          pbdev->fh = pbdev->idx;
> >          QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
> >          g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
> > +    } else {
> > +        error_setg(errp, "s390: device plug request for not supported device"
> > +                   " type: %s", object_get_typename(OBJECT(dev)));
> 
> Maybe make this "s390/pci: plugging device type <%s> is not supported"?
> 

Under what circumstances could/does this happen? I mean how can this
be triggered by the user?

Regards,
Halil

> >      }
> >  }
> >  
> > @@ -956,6 +959,10 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_S390_PCI_DEVICE)) {
> >          pbdev = S390_PCI_DEVICE(dev);
> >          pci_dev = pbdev->pdev;
> > +    } else {
> > +        error_setg(errp, "s390: device unplug request for not supported device"
> > +                   " type: %s", object_get_typename(OBJECT(dev)));
> 
> Halil has a point when he suggests an assert here. If we fence the
> device type in the plug function, I can't think of a way we would end
> up here.
> 
> > +        return;
> >      }
> >  
> >      switch (pbdev->state) {
> 
> 

Re: [Qemu-devel] [qemu-s390x] [PATCH v2] s390: avoid potential null dereference in s390_pcihost_unplug()

[Cornelia Huck]

On Fri, 4 Jan 2019 16:05:15 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 4 Jan 2019 15:10:05 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu,  3 Jan 2019 07:16:12 -0800
> > Li Qiang <liq3ea@163.com> wrote:
> >   
> > > When getting the 'pbdev', the if...else has no default branch.
> > > From Coverity, the 'pbdev' maybe null when the 'dev' is not
> > > the TYPE_PCI_BRIDGE/TYPE_PCI_DEVICE/TYPE_S390_PCI_DEVICE.
> > > This patch adds a default branch for device plug and unplug.
> > > 
> > > Spotted by Coverity: CID 1398593
> > > 
> > > Signed-off-by: Li Qiang <liq3ea@163.com>
> > > ---
> > > Adds a default branch for device plug per Cornelia's review.
> > > 
> > >  hw/s390x/s390-pci-bus.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > > index 15759b6514..fe48a36ff6 100644
> > > --- a/hw/s390x/s390-pci-bus.c
> > > +++ b/hw/s390x/s390-pci-bus.c
> > > @@ -912,6 +912,9 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          pbdev->fh = pbdev->idx;
> > >          QTAILQ_INSERT_TAIL(&s->zpci_devs, pbdev, link);
> > >          g_hash_table_insert(s->zpci_table, &pbdev->idx, pbdev);
> > > +    } else {
> > > +        error_setg(errp, "s390: device plug request for not supported device"
> > > +                   " type: %s", object_get_typename(OBJECT(dev)));  
> > 
> > Maybe make this "s390/pci: plugging device type <%s> is not supported"?
> >   
> 
> Under what circumstances could/does this happen? I mean how can this
> be triggered by the user?

Probably only if a new type that can be plugged has been added, but the
s390 pci code has not been updated. We could also assert, not sure what
would make it easier to figure out what went wrong.