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

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

[Tuo Li]

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@gmail.com>
---
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");
+	if (isa_dev)
+		dev_dbg(&isa_dev->dev, "Type-F DMA livelock erratum (C3 disabled)\n");
 
 	return 0;
 }
-- 
2.43.0

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

[Rafael J. Wysocki]

On Sun, Jan 11, 2026 at 5:32 PM Tuo Li <islituo@gmail.com> 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@gmail.com>
> ---
> 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");
> +       if (isa_dev)
> +               dev_dbg(&isa_dev->dev, "Type-F DMA livelock erratum (C3 disabled)\n");
>
>         return 0;
>  }
> --

Applied as 6.20 material, thanks!

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

[Guenter Roeck]

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@gmail.com>

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
> 

[PATCH v1] ACPI: processor: Fix previous acpi_processor_errata_piix4() fix

[Rafael J. Wysocki]

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@gmail.com>
> 
> 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@intel.com>
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@roeck-us.net>
Closes: https://lore.kernel.org/linux-acpi/938e2206-def5-4b7a-9b2c-d1fd37681d8a@roeck-us.net/
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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;
 }
 

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@roeck-us.net> 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@gmail.com>
>
> 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

Re: [PATCH v1] ACPI: processor: Fix previous acpi_processor_errata_piix4() fix

[Guenter Roeck]

On 3/17/26 13:39, Rafael J. Wysocki wrote:
> 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@gmail.com>
>>
>> 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@intel.com>
> 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@roeck-us.net>
> Closes: https://lore.kernel.org/linux-acpi/938e2206-def5-4b7a-9b2c-d1fd37681d8a@roeck-us.net/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   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;
>   }
>   
> 
> 
>