[PATCH v1] ACPI: processor: Fix previous acpi_processor_errata_piix4() fix
From: Rafael J. Wysocki
Date: Tue Mar 17 2026 - 16:39:15 EST
On Tuesday, March 17, 2026 6:26:59 PM CET Guenter Roeck wrote:
> Hi,
>
> On Mon, Jan 12, 2026 at 12:32:14AM +0800, Tuo Li wrote:
> > In acpi_processor_errata_piix4(), the pointer dev is first assigned an IDE
> > device and then reassigned an ISA device:
> >
> > dev = pci_get_subsys(..., PCI_DEVICE_ID_INTEL_82371AB, ...);
> > dev = pci_get_subsys(..., PCI_DEVICE_ID_INTEL_82371AB_0, ...);
> >
> > If the first lookup succeeds but the second fails, dev becomes NULL. This
> > leads to a potential null-pointer dereference when dev_dbg() is called:
> >
> > if (errata.piix4.bmisx)
> > dev_dbg(&dev->dev, ...);
> >
> > To prevent this, use two temporary pointers and retrieve each device
> > independently, avoiding overwriting dev with a possible NULL value.
> >
> >
> > Signed-off-by: Tuo Li <islituo@xxxxxxxxx>
>
> AI review feedback inline below.
>
> I don't know if the device lifetime issue is real, but the now unconditional
> debug message is definitely a functional change and potentially misleading.
>
> Either case, why not just move the debug messages into the code setting
> errata.piix4.bmisx and errata.piix4.fdma ?
>
> Thanks,
> Guenter
>
> > ---
> > v3:
> > * Initialize the new variables to NULL and drop redundant checks.
> > Thanks Rafael J. Wysocki for helpful advice.
> > v2:
> > * Add checks for ide_dev and isa_dev before dev_dbg()
> > Thanks Rafael J. Wysocki for helpful advice.
> > ---
> > drivers/acpi/acpi_processor.c | 27 ++++++++++++++-------------
> > 1 file changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> > index 7ec1dc04fd11..de256e3adeed 100644
> > --- a/drivers/acpi/acpi_processor.c
> > +++ b/drivers/acpi/acpi_processor.c
> > @@ -50,6 +50,7 @@ static int acpi_processor_errata_piix4(struct pci_dev *dev)
> > {
> > u8 value1 = 0;
> > u8 value2 = 0;
> > + struct pci_dev *ide_dev = NULL, *isa_dev = NULL;
> >
> >
> > if (!dev)
> > @@ -107,12 +108,12 @@ static int acpi_processor_errata_piix4(struct pci_dev *dev)
> > * each IDE controller's DMA status to make sure we catch all
> > * DMA activity.
> > */
> > - dev = pci_get_subsys(PCI_VENDOR_ID_INTEL,
> > + ide_dev = pci_get_subsys(PCI_VENDOR_ID_INTEL,
> > PCI_DEVICE_ID_INTEL_82371AB,
> > PCI_ANY_ID, PCI_ANY_ID, NULL);
> > - if (dev) {
> > - errata.piix4.bmisx = pci_resource_start(dev, 4);
> > - pci_dev_put(dev);
> > + if (ide_dev) {
> > + errata.piix4.bmisx = pci_resource_start(ide_dev, 4);
> > + pci_dev_put(ide_dev);
> > }
> >
> > /*
> > @@ -124,24 +125,24 @@ static int acpi_processor_errata_piix4(struct pci_dev *dev)
> > * disable C3 support if this is enabled, as some legacy
> > * devices won't operate well if fast DMA is disabled.
> > */
> > - dev = pci_get_subsys(PCI_VENDOR_ID_INTEL,
> > + isa_dev = pci_get_subsys(PCI_VENDOR_ID_INTEL,
> > PCI_DEVICE_ID_INTEL_82371AB_0,
> > PCI_ANY_ID, PCI_ANY_ID, NULL);
> > - if (dev) {
> > - pci_read_config_byte(dev, 0x76, &value1);
> > - pci_read_config_byte(dev, 0x77, &value2);
> > + if (isa_dev) {
> > + pci_read_config_byte(isa_dev, 0x76, &value1);
> > + pci_read_config_byte(isa_dev, 0x77, &value2);
> > if ((value1 & 0x80) || (value2 & 0x80))
> > errata.piix4.fdma = 1;
> > - pci_dev_put(dev);
> > + pci_dev_put(isa_dev);
> > }
> >
> > break;
> > }
> >
> > - if (errata.piix4.bmisx)
> > - dev_dbg(&dev->dev, "Bus master activity detection (BM-IDE) erratum enabled\n");
> > - if (errata.piix4.fdma)
> > - dev_dbg(&dev->dev, "Type-F DMA livelock erratum (C3 disabled)\n");
> > + if (ide_dev)
> > + dev_dbg(&ide_dev->dev, "Bus master activity detection (BM-IDE) erratum enabled\n");
>
> Is it safe to dereference ide_dev here? It looks like pci_dev_put(ide_dev)
> was already called above, which drops the reference and might lead to a
> use-after-free.
>
> Also, does this change the logging behavior? The original code checked
> errata.piix4.bmisx before logging, but now it logs as long as ide_dev
> was found, even if the erratum is not actually enabled.
Well, it has a point.
The patch below should fix this.
---
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
Subject: [PATCH v1] ACPI: processor: Fix previous acpi_processor_errata_piix4() fix
After commi f132e089fe89 ("ACPI: processor: Fix NULL-pointer dereference
in acpi_processor_errata_piix4()"), device pointers may be dereferenced
after dropping references to the device objects pointed to by them,
which may cause a use-after-free to occur.
Moreover, debug messages about enabling the errata may be printed
if the errata flags corresponding to them are unset.
Address all of these issues by moving message printing to the points
in the code where the errata flags are set.
Fixes: f132e089fe89 ("ACPI: processor: Fix NULL-pointer dereference in acpi_processor_errata_piix4()")
Reported-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Closes: https://lore.kernel.org/linux-acpi/938e2206-def5-4b7a-9b2c-d1fd37681d8a@xxxxxxxxxxxx/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
drivers/acpi/acpi_processor.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -113,6 +113,10 @@ static int acpi_processor_errata_piix4(s
PCI_ANY_ID, PCI_ANY_ID, NULL);
if (ide_dev) {
errata.piix4.bmisx = pci_resource_start(ide_dev, 4);
+ if (errata.piix4.bmisx)
+ dev_dbg(&ide_dev->dev,
+ "Bus master activity detection (BM-IDE) erratum enabled\n");
+
pci_dev_put(ide_dev);
}
@@ -131,20 +135,17 @@ static int acpi_processor_errata_piix4(s
if (isa_dev) {
pci_read_config_byte(isa_dev, 0x76, &value1);
pci_read_config_byte(isa_dev, 0x77, &value2);
- if ((value1 & 0x80) || (value2 & 0x80))
+ if ((value1 & 0x80) || (value2 & 0x80)) {
errata.piix4.fdma = 1;
+ dev_dbg(&isa_dev->dev,
+ "Type-F DMA livelock erratum (C3 disabled)\n");
+ }
pci_dev_put(isa_dev);
}
break;
}
- if (ide_dev)
- dev_dbg(&ide_dev->dev, "Bus master activity detection (BM-IDE) erratum enabled\n");
-
- if (isa_dev)
- dev_dbg(&isa_dev->dev, "Type-F DMA livelock erratum (C3 disabled)\n");
-
return 0;
}