iommu/amd: Prevent possible null pointer dereference and infinite loop

The check for !dev_data->domain in __detach_device() emits a warning and
returns. The calling code in detach_device() dereferences dev_data->domain
afterwards unconditionally, so in case that dev_data->domain is NULL the
warning will be immediately followed by a NULL pointer dereference.

The calling code in cleanup_domain() loops infinite when !dev_data->domain
and the check in __detach_device() returns immediately because dev_list is
not changed.

do_detach() duplicates this check without throwing a warning.

Move the check with the explanation of the do_detach() code into the caller
detach_device() and return immediately. Throw an error, when hitting the
condition in cleanup_domain().

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Joerg Roedel <jroedel@suse.de>

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index ec5faf6..92241ff 100644 (file)
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1915,15 +1915,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
        struct amd_iommu *iommu;
        u16 alias;
 
-       /*
-        * First check if the device is still attached. It might already
-        * be detached from its domain because the generic
-        * iommu_detach_group code detached it and we try again here in
-        * our alias handling.
-        */
-       if (!dev_data->domain)
-               return;
-
        iommu = amd_iommu_rlookup_table[dev_data->devid];
        alias = dev_data->alias;
 
@@ -2128,9 +2119,6 @@ static void __detach_device(struct iommu_dev_data *dev_data)
         */
        WARN_ON(!irqs_disabled());
 
-       if (WARN_ON(!dev_data->domain))
-               return;
-
        domain = dev_data->domain;
 
        spin_lock(&domain->lock);
@@ -2152,6 +2140,15 @@ static void detach_device(struct device *dev)
        dev_data = get_dev_data(dev);
        domain   = dev_data->domain;
 
+       /*
+        * First check if the device is still attached. It might already
+        * be detached from its domain because the generic
+        * iommu_detach_group code detached it and we try again here in
+        * our alias handling.
+        */
+       if (WARN_ON(!dev_data->domain))
+               return;
+
        /* lock device table */
        spin_lock_irqsave(&amd_iommu_devtable_lock, flags);
        __detach_device(dev_data);
@@ -2797,6 +2794,7 @@ static void cleanup_domain(struct protection_domain *domain)
        while (!list_empty(&domain->dev_list)) {
                entry = list_first_entry(&domain->dev_list,
                                         struct iommu_dev_data, list);
+               BUG_ON(!entry->domain);
                __detach_device(entry);
        }