Re: [PATCH] hw/acpi/pcihp: validate bsel property of the bus before unplugging device

[Michael S. Tsirkin]

On Sat, Aug 21, 2021 at 08:35:35PM +0530, Ani Sinha wrote:
> Bsel property of the pci bus indicates whether the bus supports acpi hotplug.
> We need to validate the presence of this property before performing any 
> hotplug
> related callback operations. Currently validation of the existence of this
> property was absent from acpi_pcihp_device_unplug_cb() function but is present
> in other hotplug/unplug callback functions. Hence, this change adds the 
> missing
> check for the above function.
> 
> Signed-off-by: Ani Sinha <ani@anisinha.ca>

I queued this but I have a general question:
are all these errors logged with LOG_GUEST_ERROR?
Because if not we have a security problem.
I also note that bsel is an internal property,
I am not sure we should be printing this to users,
it might just confuse them.

Same question for all the other places validating bsel.

> ---
>  hw/acpi/pcihp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 0fd0c1d811..9982815a87 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -372,9 +372,15 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler 
> *hotplug_dev, AcpiPciHpState *s,
>                                   DeviceState *dev, Error **errp)
>  {
>      PCIDevice *pdev = PCI_DEVICE(dev);
> +    int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> +
> +    trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn), bsel);
>  
> -    trace_acpi_pci_unplug(PCI_SLOT(pdev->devfn),
> -                          acpi_pcihp_get_bsel(pci_get_bus(pdev)));
> +    if (bsel < 0) {
> +        error_setg(errp, "Unsupported bus. Bus doesn't have property '"
> +                   ACPI_PCIHP_PROP_BSEL "' set");
> +        return;
> +    }
>  
>      /*
>       * clean up acpi-index so it could reused by another device
> -- 
> 2.25.1