[Intel-gfx] [PATCH] drm/i915: fix pch pci device enumeration

[Chris Wilson]

On Fri, Feb 14, 2014 at 07:23:32PM +0200, Imre Deak wrote:
> pci_get_class(class, from) drops the refcount for 'from', so the
> extra pci_dev_put we do on it will result in a use after free bug
> sometime later starting with the WARN below.

That's a very nice find.

But you can tidy this loop up even more...

> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 2d05d7c..4e4fc0c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -347,7 +347,6 @@ void intel_detect_pch(struct drm_device *dev)
>  	 */
>  	pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
>  	while (pch) {

...by noting that what you want here is
while ((pch = pci_get_class(ISA<<8, pch)) {

> -		struct pci_dev *curr = pch;
>  		if (pch->vendor == PCI_VENDOR_ID_INTEL) {
>  			unsigned short id;
>  			id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
> @@ -385,15 +384,15 @@ void intel_detect_pch(struct drm_device *dev)
>  			} else {
>  				goto check_next;
>  			}
> -			pci_dev_put(pch);
>  			break;
>  		}
>  check_next:
> -		pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
> -		pci_dev_put(curr);
> +		pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch);
>  	}
>  	if (!pch)
>  		DRM_DEBUG_KMS("No PCH found?\n");
> +	else
> +		pci_dev_put(pch);
>  }


diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2d05d7c..9962aef 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -324,7 +324,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
 void intel_detect_pch(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct pci_dev *pch;
+	struct pci_dev *pch = NULL;
 
 	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
 	 * (which really amounts to a PCH but no South Display).
@@ -345,12 +345,9 @@ void intel_detect_pch(struct drm_device *dev)
 	 * all the ISA bridge devices and check for the first match, instead
 	 * of only checking the first one.
 	 */
-	pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, NULL);
-	while (pch) {
-		struct pci_dev *curr = pch;
+	while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
 		if (pch->vendor == PCI_VENDOR_ID_INTEL) {
-			unsigned short id;
-			id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
+			unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
 			dev_priv->pch_id = id;
 
 			if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
@@ -382,18 +379,15 @@ void intel_detect_pch(struct drm_device *dev)
 				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
 				WARN_ON(!IS_HASWELL(dev));
 				WARN_ON(!IS_ULT(dev));
-			} else {
-				goto check_next;
-			}
+			} else
+				continue;
+
 			pci_dev_put(pch);
 			break;
 		}
-check_next:
-		pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, curr);
-		pci_dev_put(curr);
 	}
 	if (!pch)
-		DRM_DEBUG_KMS("No PCH found?\n");
+		DRM_DEBUG_KMS("No PCH found.\n");
 }
 

-- 
Chris Wilson, Intel Open Source Technology Centre