Re: [PATCH v3] ACPI: processor: Fix a possible null-pointer dereference in acpi_processor_errata_piix4() when debug messages are enabled

[Tuo Li]

Hi,


On Wed, Mar 18, 2026 at 1:27 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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.
>
> > +     if (isa_dev)
> > +             dev_dbg(&isa_dev->dev, "Type-F DMA livelock erratum (C3 disabled)\n");
>
> Similarly, could this cause a use-after-free on isa_dev after
> pci_dev_put(isa_dev) was called?
>
> Additionally, does this unconditionally print the "Type-F DMA livelock"
> message if the ISA controller is present, skipping the original check
> for errata.piix4.fdma?
>
> Since the original `dev` parameter is no longer reassigned, would it be
> better to keep using `&dev->dev` and the original errata checks here to log
> against the original PIIX4 ACPI controller?
>
> >
> >       return 0;
> >  }
> > --
> > 2.43.0
> >

Thanks for pointing this out, and I am sorry for the oversight.

I think these issues are being fixed in a follow-up patch:
https://lore.kernel.org/5975693.DvuYhMxLoT@rafael.j.wysocki/

Thanks,
Tuo