[PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Ani Sinha]

When ACPI hotplug for the root bus is disabled, the bsel property for that
bus is not set. Please see the following commit:

3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus").

As a result, when acpi_pcihp_find_hotplug_bus() is called
with bsel set to 0, it may return the root bus. This would be wrong since the
root bus is not hotpluggable. In general, this can potentially happen to other
buses as well.
In this patch, we fix the issue in this function by checking if the bus returned
by the function is actually hotpluggable. If not, we simply return NULL. This
avoids the scenario where we are actually returning a non-hotpluggable bus.

Signed-off-by: Ani Sinha <ani@anisinha.ca>
---
 hw/acpi/pcihp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 39b1f74442..f148e73c89 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -147,6 +147,21 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
     if (!bsel && !find.bus) {
         find.bus = s->root;
     }
+
+    /*
+     * Check if find.bus is actually hotpluggable. If bsel is set to
+     * NULL for example on the root bus in order to make it
+     * non-hotpluggable, find.bus will match the root bus when bsel
+     * is 0. See acpi_pcihp_test_hotplug_bus() above. Since the
+     * bus is not hotpluggable however, we should not select the bus.
+     * Instead, we should set find.bus to NULL in that case. In the check
+     * below, we generalize this case for all buses, not just the root bus.
+     * The callers of this function check for a null return value and
+     * handle them appropriately.
+     */
+    if (!qbus_is_hotpluggable(BUS(find.bus))) {
+        find.bus = NULL;
+    }
     return find.bus;
 }
 
-- 
2.17.1

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

Can someone please review this?
On Aug 29, 2020, 13:42 +0530, Ani Sinha <ani@anisinha.ca>, wrote:
> When ACPI hotplug for the root bus is disabled, the bsel property for that
> bus is not set. Please see the following commit:
>
> 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus").
>
> As a result, when acpi_pcihp_find_hotplug_bus() is called
> with bsel set to 0, it may return the root bus. This would be wrong since the
> root bus is not hotpluggable. In general, this can potentially happen to other
> buses as well.
> In this patch, we fix the issue in this function by checking if the bus returned
> by the function is actually hotpluggable. If not, we simply return NULL. This
> avoids the scenario where we are actually returning a non-hotpluggable bus.
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
> ---
> hw/acpi/pcihp.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 39b1f74442..f148e73c89 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -147,6 +147,21 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
> if (!bsel && !find.bus) {
> find.bus = s->root;
> }
> +
> + /*
> + * Check if find.bus is actually hotpluggable. If bsel is set to
> + * NULL for example on the root bus in order to make it
> + * non-hotpluggable, find.bus will match the root bus when bsel
> + * is 0. See acpi_pcihp_test_hotplug_bus() above. Since the
> + * bus is not hotpluggable however, we should not select the bus.
> + * Instead, we should set find.bus to NULL in that case. In the check
> + * below, we generalize this case for all buses, not just the root bus.
> + * The callers of this function check for a null return value and
> + * handle them appropriately.
> + */
> + if (!qbus_is_hotpluggable(BUS(find.bus))) {
> + find.bus = NULL;
> + }
> return find.bus;
> }
>
> --
> 2.17.1
>

[-- Attachment #2: Type: text/html, Size: 2561 bytes --]

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Michael S. Tsirkin]

On Sat, Aug 29, 2020 at 01:42:33PM +0530, Ani Sinha wrote:
> When ACPI hotplug for the root bus is disabled, the bsel property for that
> bus is not set. Please see the following commit:
> 
> 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus").
> 
> As a result, when acpi_pcihp_find_hotplug_bus() is called
> with bsel set to 0, it may return the root bus. This would be wrong since the
> root bus is not hotpluggable. In general, this can potentially happen to other
> buses as well.
> In this patch, we fix the issue in this function by checking if the bus returned
> by the function is actually hotpluggable. If not, we simply return NULL. This
> avoids the scenario where we are actually returning a non-hotpluggable bus.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

What exactly are the consequences though?


> ---
>  hw/acpi/pcihp.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 39b1f74442..f148e73c89 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -147,6 +147,21 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
>      if (!bsel && !find.bus) {
>          find.bus = s->root;
>      }
> +
> +    /*
> +     * Check if find.bus is actually hotpluggable. If bsel is set to
> +     * NULL for example on the root bus in order to make it
> +     * non-hotpluggable, find.bus will match the root bus when bsel
> +     * is 0. See acpi_pcihp_test_hotplug_bus() above. Since the
> +     * bus is not hotpluggable however, we should not select the bus.
> +     * Instead, we should set find.bus to NULL in that case. In the check
> +     * below, we generalize this case for all buses, not just the root bus.
> +     * The callers of this function check for a null return value and
> +     * handle them appropriately.
> +     */
> +    if (!qbus_is_hotpluggable(BUS(find.bus))) {
> +        find.bus = NULL;
> +    }
>      return find.bus;
>  }
>  
> -- 
> 2.17.1

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

 On Sep 3, 2020, 15:35 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:

On Sat, Aug 29, 2020 at 01:42:33PM +0530, Ani Sinha wrote:

When ACPI hotplug for the root bus is disabled, the bsel property for that

bus is not set. Please see the following commit:


3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on
the root bus").


As a result, when acpi_pcihp_find_hotplug_bus() is called

with bsel set to 0, it may return the root bus. This would be wrong since
the

root bus is not hotpluggable. In general, this can potentially happen to
other

buses as well.

In this patch, we fix the issue in this function by checking if the bus
returned

by the function is actually hotpluggable. If not, we simply return NULL.
This

avoids the scenario where we are actually returning a non-hotpluggable bus.


Signed-off-by: Ani Sinha <ani@anisinha.ca>


What exactly are the consequences though?


The root bus might get ejected by the user when it should not if the user
does the following:

outl 0xae10 0
outl 0xae08 your_slot

Please see Julia’s comment:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg734548.html

[-- Attachment #2: Type: text/html, Size: 6691 bytes --]

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Ani Sinha]

On Thu, Sep 3, 2020 at 3:41 PM Ani Sinha <ani@anisinha.ca> wrote:
>
> On Sep 3, 2020, 15:35 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
>
> On Sat, Aug 29, 2020 at 01:42:33PM +0530, Ani Sinha wrote:
>
> When ACPI hotplug for the root bus is disabled, the bsel property for that
>
> bus is not set. Please see the following commit:
>
>
> 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on the root bus").
>
>
> As a result, when acpi_pcihp_find_hotplug_bus() is called
>
> with bsel set to 0, it may return the root bus. This would be wrong since the
>
> root bus is not hotpluggable. In general, this can potentially happen to other
>
> buses as well.
>
> In this patch, we fix the issue in this function by checking if the bus returned
>
> by the function is actually hotpluggable. If not, we simply return NULL. This
>
> avoids the scenario where we are actually returning a non-hotpluggable bus.
>
>
> Signed-off-by: Ani Sinha <ani@anisinha.ca>
>
>
> What exactly are the consequences though?
>
>
> The root bus

s/root bus/any device on the root bus
sorry.

might get ejected by the user when it should not if the user does the following:
>
> outl 0xae10 0
> outl 0xae08 your_slot
>
> Please see Julia’s comment:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg734548.html
>

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Michael S. Tsirkin]

On Thu, Sep 03, 2020 at 03:41:13PM +0530, Ani Sinha wrote:
> On Sep 3, 2020, 15:35 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> 
>     On Sat, Aug 29, 2020 at 01:42:33PM +0530, Ani Sinha wrote:
> 
>         When ACPI hotplug for the root bus is disabled, the bsel property for
>         that
> 
>         bus is not set. Please see the following commit:
> 
> 
> 
>         3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug
>         on the root bus").
> 
> 
> 
>         As a result, when acpi_pcihp_find_hotplug_bus() is called
> 
>         with bsel set to 0, it may return the root bus. This would be wrong
>         since the
> 
>         root bus is not hotpluggable. In general, this can potentially happen
>         to other
> 
>         buses as well.
> 
>         In this patch, we fix the issue in this function by checking if the bus
>         returned
> 
>         by the function is actually hotpluggable. If not, we simply return
>         NULL. This
> 
>         avoids the scenario where we are actually returning a non-hotpluggable
>         bus.
> 
> 
> 
>         Signed-off-by: Ani Sinha <ani@anisinha.ca>
> 
> 
> 
>     What exactly are the consequences though?
> 
> 
> The root bus might get ejected by the user when it should not if the user does
> the following:
> 
> outl 0xae10 0
> outl 0xae08 your_slot
> 
> Please see Julia’s comment:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg734548.html

OK so patch looks good, but please add all this in the commit log.

-- 
MST

Re: [PATCH] Fix a gap where acpi_pcihp_find_hotplug_bus() returns a non-hotpluggable bus

[Ani Sinha]

On Thu, Sep 3, 2020 at 3:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Sep 03, 2020 at 03:41:13PM +0530, Ani Sinha wrote:
> > On Sep 3, 2020, 15:35 +0530, Michael S. Tsirkin <mst@redhat.com>, wrote:
> >
> >     On Sat, Aug 29, 2020 at 01:42:33PM +0530, Ani Sinha wrote:
> >
> >         When ACPI hotplug for the root bus is disabled, the bsel property for
> >         that
> >
> >         bus is not set. Please see the following commit:
> >
> >
> >
> >         3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug
> >         on the root bus").
> >
> >
> >
> >         As a result, when acpi_pcihp_find_hotplug_bus() is called
> >
> >         with bsel set to 0, it may return the root bus. This would be wrong
> >         since the
> >
> >         root bus is not hotpluggable. In general, this can potentially happen
> >         to other
> >
> >         buses as well.
> >
> >         In this patch, we fix the issue in this function by checking if the bus
> >         returned
> >
> >         by the function is actually hotpluggable. If not, we simply return
> >         NULL. This
> >
> >         avoids the scenario where we are actually returning a non-hotpluggable
> >         bus.
> >
> >
> >
> >         Signed-off-by: Ani Sinha <ani@anisinha.ca>
> >
> >
> >
> >     What exactly are the consequences though?
> >
> >
> > The root bus might get ejected by the user when it should not if the user does
> > the following:
> >
> > outl 0xae10 0
> > outl 0xae08 your_slot
> >
> > Please see Julia’s comment:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg734548.html
>
> OK so patch looks good, but please add all this in the commit log.

Done. V2 sent.