QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* acpi_pcihp_eject_slot() bug if passed 'slots == 0'
@ 2020-03-26 11:52 Peter Maydell
  2020-03-26 12:29 ` Igor Mammedov
  2020-03-26 13:23 ` Igor Mammedov
  0 siblings, 2 replies; 8+ messages in thread
From: Peter Maydell @ 2020-03-26 11:52 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Igor Mammedov, Michael S. Tsirkin

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 11:52 acpi_pcihp_eject_slot() bug if passed 'slots == 0' Peter Maydell
@ 2020-03-26 12:29 ` Igor Mammedov
  2020-03-26 12:50   ` Igor Mammedov
  2020-03-26 13:23 ` Igor Mammedov
  1 sibling, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2020-03-26 12:29 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Michael S. Tsirkin

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
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 12:29 ` Igor Mammedov
@ 2020-03-26 12:50   ` Igor Mammedov
  2020-03-26 13:29     ` Michael S. Tsirkin
  0 siblings, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2020-03-26 12:50 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Michael S. Tsirkin

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 11:52 acpi_pcihp_eject_slot() bug if passed 'slots == 0' Peter Maydell
  2020-03-26 12:29 ` Igor Mammedov
@ 2020-03-26 13:23 ` Igor Mammedov
  2020-03-26 13:28   ` Michael S. Tsirkin
  1 sibling, 1 reply; 8+ messages in thread
From: Igor Mammedov @ 2020-03-26 13:23 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Michael S. Tsirkin

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
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 13:23 ` Igor Mammedov
@ 2020-03-26 13:28   ` Michael S. Tsirkin
  2020-03-26 13:31     ` Michael S. Tsirkin
  0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-03-26 13:28 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Peter Maydell, QEMU Developers

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 12:50   ` Igor Mammedov
@ 2020-03-26 13:29     ` Michael S. Tsirkin
  0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-03-26 13:29 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Peter Maydell, QEMU Developers

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 13:28   ` Michael S. Tsirkin
@ 2020-03-26 13:31     ` Michael S. Tsirkin
  2020-03-26 13:40       ` Igor Mammedov
  0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-03-26 13:31 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Peter Maydell, QEMU Developers

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: acpi_pcihp_eject_slot() bug if passed 'slots == 0'
  2020-03-26 13:31     ` Michael S. Tsirkin
@ 2020-03-26 13:40       ` Igor Mammedov
  0 siblings, 0 replies; 8+ messages in thread
From: Igor Mammedov @ 2020-03-26 13:40 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Peter Maydell, QEMU Developers

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 11:52 acpi_pcihp_eject_slot() bug if passed 'slots == 0' Peter Maydell
2020-03-26 12:29 ` Igor Mammedov
2020-03-26 12:50   ` Igor Mammedov
2020-03-26 13:29     ` Michael S. Tsirkin
2020-03-26 13:23 ` Igor Mammedov
2020-03-26 13:28   ` Michael S. Tsirkin
2020-03-26 13:31     ` Michael S. Tsirkin
2020-03-26 13:40       ` Igor Mammedov

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git