* 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 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 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 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, other threads:[~2020-03-26 13:41 UTC | newest] 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
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).