acpi_pcihp_eject_slot() bug if passed 'slots == 0'

acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Peter Maydell]

Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
is passed a zero 'slots' argument then ctz32(slots) will return 32,
and then the code that does '1U << slot' is C undefined behaviour
because it's an oversized shift. (This is CID 1421896.)

Since the pci_write() function in this file can call
acpi_pcihp_eject_slot() with an arbitrary value from the guest,
I think we need to handle 'slots == 0' safely. But what should
the behaviour be?

thanks
-- PMM

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Igor Mammedov]

On Thu, 26 Mar 2020 11:52:36 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> is passed a zero 'slots' argument then ctz32(slots) will return 32,
> and then the code that does '1U << slot' is C undefined behaviour
> because it's an oversized shift. (This is CID 1421896.)
> 
> Since the pci_write() function in this file can call
> acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> I think we need to handle 'slots == 0' safely. But what should
> the behaviour be?

0 is not valid value, we should ignore and return early in this case
like we do with bsel. I'll post a path shortly.

> 
> thanks
> -- PMM
> 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Igor Mammedov]

On Thu, 26 Mar 2020 13:29:01 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 26 Mar 2020 11:52:36 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> > is passed a zero 'slots' argument then ctz32(slots) will return 32,
> > and then the code that does '1U << slot' is C undefined behaviour
> > because it's an oversized shift. (This is CID 1421896.)
> > 
> > Since the pci_write() function in this file can call
> > acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> > I think we need to handle 'slots == 0' safely. But what should
> > the behaviour be?  
> 
> 0 is not valid value, we should ignore and return early in this case
> like we do with bsel. I'll post a path shortly.
well, looking more that is only true for main bus, for bridges it can be
slot number can be zero, then AML left shifts it and writes into B0EJ
which traps into pci_write(, data) and that is supposed to eject
slot 0 according to guest(AML).

Michael,
what's your take on it?

> 
> > 
> > thanks
> > -- PMM
> >   
> 
> 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Igor Mammedov]

On Thu, 26 Mar 2020 11:52:36 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> is passed a zero 'slots' argument then ctz32(slots) will return 32,
> and then the code that does '1U << slot' is C undefined behaviour
> because it's an oversized shift. (This is CID 1421896.)
> 
> Since the pci_write() function in this file can call
> acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> I think we need to handle 'slots == 0' safely. But what should
> the behaviour be?

it also uncovers a bug, where we are not able to eject slot 0 on bridge,
can be reproduced with:

 -enable-kvm -m 4G -device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device virtio-net-pci,bus=pci.1,addr=0,id=netdev12

(monitor) device_del netdev12
(monitor) qtree # still shows the device

reason is that acpi_pcihp_eject_slot()
   if (PCI_SLOT(dev->devfn) == slot) { # doesn't  match (0 != 32)

so device is not deleted

> thanks
> -- PMM
> 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Michael S. Tsirkin]

On Thu, Mar 26, 2020 at 02:23:17PM +0100, Igor Mammedov wrote:
> On Thu, 26 Mar 2020 11:52:36 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> > is passed a zero 'slots' argument then ctz32(slots) will return 32,
> > and then the code that does '1U << slot' is C undefined behaviour
> > because it's an oversized shift. (This is CID 1421896.)
> > 
> > Since the pci_write() function in this file can call
> > acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> > I think we need to handle 'slots == 0' safely. But what should
> > the behaviour be?
> 
> it also uncovers a bug, where we are not able to eject slot 0 on bridge,


And that is by design. A standard PCI SHPC register can support up to 31
hotpluggable slots. So we chose slot 0 as non hotpluggable.
It's consistent across SHPC, PCI-E, so I made ACPI match.
You can't hot-add it either.

> can be reproduced with:
> 
>  -enable-kvm -m 4G -device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device virtio-net-pci,bus=pci.1,addr=0,id=netdev12
> 
> (monitor) device_del netdev12
> (monitor) qtree # still shows the device
> 
> reason is that acpi_pcihp_eject_slot()
>    if (PCI_SLOT(dev->devfn) == slot) { # doesn't  match (0 != 32)
> 
> so device is not deleted

We should probably teach QEMU that some slots aren't hotpluggable
even if device in it is hotpluggable in theory. But that is
a separate issue.

> > thanks
> > -- PMM
> > 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Michael S. Tsirkin]

On Thu, Mar 26, 2020 at 01:50:41PM +0100, Igor Mammedov wrote:
> On Thu, 26 Mar 2020 13:29:01 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 26 Mar 2020 11:52:36 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > 
> > > Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> > > is passed a zero 'slots' argument then ctz32(slots) will return 32,
> > > and then the code that does '1U << slot' is C undefined behaviour
> > > because it's an oversized shift. (This is CID 1421896.)
> > > 
> > > Since the pci_write() function in this file can call
> > > acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> > > I think we need to handle 'slots == 0' safely. But what should
> > > the behaviour be?  
> > 
> > 0 is not valid value, we should ignore and return early in this case
> > like we do with bsel. I'll post a path shortly.
> well, looking more that is only true for main bus, for bridges it can be
> slot number can be zero,

It can but we don't allow slot zero hotplug with SHPC
so it's easier if we don't allow this with ACPI either.

> then AML left shifts it and writes into B0EJ
> which traps into pci_write(, data) and that is supposed to eject
> slot 0 according to guest(AML).
> 
> Michael,
> what's your take on it?
> 
> > 
> > > 
> > > thanks
> > > -- PMM
> > >   
> > 
> > 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Michael S. Tsirkin]

On Thu, Mar 26, 2020 at 09:28:27AM -0400, Michael S. Tsirkin wrote:
> On Thu, Mar 26, 2020 at 02:23:17PM +0100, Igor Mammedov wrote:
> > On Thu, 26 Mar 2020 11:52:36 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > 
> > > Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> > > is passed a zero 'slots' argument then ctz32(slots) will return 32,
> > > and then the code that does '1U << slot' is C undefined behaviour
> > > because it's an oversized shift. (This is CID 1421896.)
> > > 
> > > Since the pci_write() function in this file can call
> > > acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> > > I think we need to handle 'slots == 0' safely. But what should
> > > the behaviour be?
> > 
> > it also uncovers a bug, where we are not able to eject slot 0 on bridge,
> 
> 
> And that is by design. A standard PCI SHPC register can support up to 31
> hotpluggable slots. So we chose slot 0 as non hotpluggable.
> It's consistent across SHPC, PCI-E, so I made ACPI match.

Sorry I was confused. It's a PCI thing, PCI-E does not have
slot numbers for downstream ports at all.

> You can't hot-add it either.
> 
> > can be reproduced with:
> > 
> >  -enable-kvm -m 4G -device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device virtio-net-pci,bus=pci.1,addr=0,id=netdev12
> > 
> > (monitor) device_del netdev12
> > (monitor) qtree # still shows the device
> > 
> > reason is that acpi_pcihp_eject_slot()
> >    if (PCI_SLOT(dev->devfn) == slot) { # doesn't  match (0 != 32)
> > 
> > so device is not deleted
> 
> We should probably teach QEMU that some slots aren't hotpluggable
> even if device in it is hotpluggable in theory. But that is
> a separate issue.
> 
> > > thanks
> > > -- PMM
> > > 

Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'

[Igor Mammedov]

On Thu, 26 Mar 2020 09:31:09 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Mar 26, 2020 at 09:28:27AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Mar 26, 2020 at 02:23:17PM +0100, Igor Mammedov wrote:  
> > > On Thu, 26 Mar 2020 11:52:36 +0000
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > >   
> > > > Hi; Coverity spots that if hw/acpi/pcihp.c:acpi_pcihp_eject_slot()
> > > > is passed a zero 'slots' argument then ctz32(slots) will return 32,
> > > > and then the code that does '1U << slot' is C undefined behaviour
> > > > because it's an oversized shift. (This is CID 1421896.)
> > > > 
> > > > Since the pci_write() function in this file can call
> > > > acpi_pcihp_eject_slot() with an arbitrary value from the guest,
> > > > I think we need to handle 'slots == 0' safely. But what should
> > > > the behaviour be?  
> > > 
> > > it also uncovers a bug, where we are not able to eject slot 0 on bridge,  
> > 
> > 
> > And that is by design. A standard PCI SHPC register can support up to 31
> > hotpluggable slots. So we chose slot 0 as non hotpluggable.
> > It's consistent across SHPC, PCI-E, so I made ACPI match.  
> 
> Sorry I was confused. It's a PCI thing, PCI-E does not have
> slot numbers for downstream ports at all.

Scratch that, it was mistake on my part where I tests with a change
that masks 0 and wrongly at that.

slot 0 on bridge can be removed just fine 

> 
> > You can't hot-add it either.
> >   
> > > can be reproduced with:
> > > 
> > >  -enable-kvm -m 4G -device pci-bridge,chassis_nr=1 -global PIIX4_PM.acpi-pci-hotplug-with-bridge-support=on -device virtio-net-pci,bus=pci.1,addr=0,id=netdev12
> > > 
> > > (monitor) device_del netdev12
> > > (monitor) qtree # still shows the device
> > > 
> > > reason is that acpi_pcihp_eject_slot()
> > >    if (PCI_SLOT(dev->devfn) == slot) { # doesn't  match (0 != 32)
> > > 
> > > so device is not deleted  
> > 
> > We should probably teach QEMU that some slots aren't hotpluggable
> > even if device in it is hotpluggable in theory. But that is
> > a separate issue.
> >   
> > > > thanks
> > > > -- PMM
> > > >   
>