[PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev()

[PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

acpi_get_pci_dev() assumes that every handle it finds in the ACPI CA
name space, between given device handle and the PCI root bridge
handle, corresponds to a PCI-to-PCI bridge with an existing secondary
bus.  For this reason, when it finds a struct pci_dev object
corresponding to one of them, it doesn't check if its 'subordinate'
field is a valid pointer.

However, during resume from a sleep state, as well as at startup, we
may get a dock notification, via dock_acpi_notifier registered by
dock_init(), for a docking station device that is present in the ACPI
tables, but not physically accessible at the moment.  In that
situation, the device appears to be below a PCI bridge whose
'subordinate' field is NULL.  This, in turn, causes
acpi_get_pci_dev() to trigger a NULL pointer dereference.

To fix this issue make acpi_get_pci_dev() check if pdev->subordinate
is not NULL for every device it finds in the path between the root
bridge and the device it's supposed to get to and return NULL if the
"target" device is not really accessible.

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14129, which is a
regression from 2.6.30.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Reported-by: Danny Feng <dfeng@redhat.com>
Tested-by: chepioq <chepioq@gmail.com>
---
 drivers/acpi/pci_root.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -389,6 +389,18 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
 
 		pbus = pdev->subordinate;
 		pci_dev_put(pdev);
+
+		/*
+		 * During resume from a sleep state we can get a dock
+		 * notification for a device that is present in ACPI tables,
+		 * but not physically accessible at the moment, so tell the
+		 * caller it's not present in that case.
+		 */
+		if (!pbus) {
+			dev_info(&pdev->dev, "Secondary bus not present\n");
+			pdev = NULL;
+			break;
+		}
 	}
 out:
 	list_for_each_entry_safe(node, tmp, &device_list, node)

Re: [PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev()

[Alex Chiang]

* Rafael J. Wysocki <rjw@sisk.pl>:
> 
> Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14129, which is a
> regression from 2.6.30.

Thanks for doing this.

One small comment below (not major), but other than that:

Reviewed-by: Alex Chiang <achiang@hp.com>

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Reported-by: Danny Feng <dfeng@redhat.com>
> Tested-by: chepioq <chepioq@gmail.com>
> ---
>  drivers/acpi/pci_root.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> Index: linux-2.6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6/drivers/acpi/pci_root.c
> @@ -389,6 +389,18 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
>  
>  		pbus = pdev->subordinate;
>  		pci_dev_put(pdev);
> +
> +		/*
> +		 * During resume from a sleep state we can get a dock
> +		 * notification for a device that is present in ACPI tables,
> +		 * but not physically accessible at the moment, so tell the
> +		 * caller it's not present in that case.
> +		 */
> +		if (!pbus) {
> +			dev_info(&pdev->dev, "Secondary bus not present\n");

Should this be dev_dbg() or maybe even strike it completely?

> +			pdev = NULL;
> +			break;
> +		}
>  	}
>  out:
>  	list_for_each_entry_safe(node, tmp, &device_list, node)
> 

Re: [PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev()

[Rafael J. Wysocki]

On Tuesday 06 October 2009, Alex Chiang wrote:
> * Rafael J. Wysocki <rjw@sisk.pl>:
> > 
> > Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14129, which is a
> > regression from 2.6.30.
> 
> Thanks for doing this.
> 
> One small comment below (not major), but other than that:
> 
> Reviewed-by: Alex Chiang <achiang@hp.com>
> 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Reported-by: Danny Feng <dfeng@redhat.com>
> > Tested-by: chepioq <chepioq@gmail.com>
> > ---
> >  drivers/acpi/pci_root.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > Index: linux-2.6/drivers/acpi/pci_root.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/pci_root.c
> > +++ linux-2.6/drivers/acpi/pci_root.c
> > @@ -389,6 +389,18 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
> >  
> >  		pbus = pdev->subordinate;
> >  		pci_dev_put(pdev);
> > +
> > +		/*
> > +		 * During resume from a sleep state we can get a dock
> > +		 * notification for a device that is present in ACPI tables,
> > +		 * but not physically accessible at the moment, so tell the
> > +		 * caller it's not present in that case.
> > +		 */
> > +		if (!pbus) {
> > +			dev_info(&pdev->dev, "Secondary bus not present\n");
> 
> Should this be dev_dbg() or maybe even strike it completely?

I thought about that, but I decided not to just in order to get some idea about
how many affected systems are out there.  If that happens to be a problem for
anyone, we can simply remove the message in the future.

> 
> > +			pdev = NULL;
> > +			break;
> > +		}
> >  	}
> >  out:
> >  	list_for_each_entry_safe(node, tmp, &device_list, node)
> > 

Thanks,
Rafael

Re: [PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev()

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

[PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev() (rev. 2)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

acpi_get_pci_dev() may be called for a non-PCI device, in which case
it should return NULL.  However, it assumes that every handle it
finds in the ACPI CA name space, between given device handle and the
PCI root bridge handle, corresponds to a PCI-to-PCI bridge with an
existing secondary bus.  For this reason, when it finds a struct
pci_dev object corresponding to one of them, it doesn't check if
its 'subordinate' field is a valid pointer.  This obviously leads to
a NULL pointer dereference if acpi_get_pci_dev() is called for a
non-PCI device with a PCI parent which is not a bridge.

To fix this issue make acpi_get_pci_dev() check if pdev->subordinate
is not NULL for every device it finds on the path between the root
bridge and the device it's supposed to get to and return NULL if the
"target" device cannot be found.

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14129, which is a
regression from 2.6.30.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---

Jesse, Len,

This is a replacement for the original $subject patch (now in the Len's tree
as commit 5988eaded02e3cca2702f46efc255143468255bd).

The code was correct, but the comment and the changelog were not.  Please
use the one below instead.

Thanks,
Rafael

---
 drivers/acpi/pci_root.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -389,6 +389,17 @@ struct pci_dev *acpi_get_pci_dev(acpi_ha
 
 		pbus = pdev->subordinate;
 		pci_dev_put(pdev);
+
+		/*
+		 * This function may be called for a non-PCI device that has a
+		 * PCI parent (eg. a disk under a PCI SATA controller).  In that
+		 * case pdev->subordinate will be NULL for the parent.
+		 */
+		if (!pbus) {
+			dev_dbg(&pdev->dev, "Not a PCI-to-PCI bridge\n");
+			pdev = NULL;
+			break;
+		}
 	}
 out:
 	list_for_each_entry_safe(node, tmp, &device_list, node)

Re: [PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev() (rev. 2)

[Len Brown]

> Please use the one below instead.

done.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH] ACPI / PCI: Fix NULL pointer dereference in acpi_get_pci_dev() (rev. 2)

[Alex Chiang]

* Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> acpi_get_pci_dev() may be called for a non-PCI device, in which case
> it should return NULL.  However, it assumes that every handle it
> finds in the ACPI CA name space, between given device handle and the
> PCI root bridge handle, corresponds to a PCI-to-PCI bridge with an
> existing secondary bus.  For this reason, when it finds a struct
> pci_dev object corresponding to one of them, it doesn't check if
> its 'subordinate' field is a valid pointer.  This obviously leads to
> a NULL pointer dereference if acpi_get_pci_dev() is called for a
> non-PCI device with a PCI parent which is not a bridge.
> 
> To fix this issue make acpi_get_pci_dev() check if pdev->subordinate
> is not NULL for every device it finds on the path between the root
> bridge and the device it's supposed to get to and return NULL if the
> "target" device cannot be found.
> 
> Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14129, which is a
> regression from 2.6.30.
 
Acked-by: Alex Chiang <achiang@hp.com>

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> 
> Jesse, Len,
> 
> This is a replacement for the original $subject patch (now in the Len's tree
> as commit 5988eaded02e3cca2702f46efc255143468255bd).
> 
> The code was correct, but the comment and the changelog were not.  Please
> use the one below instead.
> 
> Thanks,
> Rafael
>