Skip to contentSkip to conversation

vfio/type1: fix a leak on error path

PATCH In review Dan Carpenter 2 participants 2 messages Jun 27, 2013

Subsystems (3)
KVM Kernel janitors VFIO driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 vfio/type1: fix a leak on error path

If vfio_unmap_unpin() returns an error then we leak "split". I've moved the allocation later in the function to fix this.

Signed-off-by Dan Carpenter <[email protected]>
drivers/vfio/vfio_iommu_type1.c +9 -12
index 98231d1..657f6a8 100644
@@ -367,7 +367,6 @@ static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
 				   size_t *size, struct vfio_dma *dma)
 {
 	size_t offset, overlap, tmp;
-	struct vfio_dma *split;
 	int ret;
 
 	if (!*size)

@@ -435,21 +434,13 @@ static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
 		return 0;
 	}
 
-	/* Split existing */
-	split = kzalloc(sizeof(*split), GFP_KERNEL);
-	if (!split)
-		return -ENOMEM;
-
 	offset = start - dma->iova;
 
 	ret = vfio_unmap_unpin(iommu, dma, start, size);
 	if (ret)
 		return ret;
-
-	if (!*size) {
-		kfree(split);
+	if (!*size)
 		return -EINVAL;
-	}
 
 	tmp = dma->size;
 

@@ -458,13 +449,19 @@ static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
 
 	/* Insert new for remainder, assuming it didn't all get unmapped */
 	if (likely(offset + *size < tmp)) {
+		struct vfio_dma *split;
+
+		/* Split existing */
+		split = kzalloc(sizeof(*split), GFP_KERNEL);
+		if (!split)
+			return -ENOMEM;
+
 		split->size = tmp - offset - *size;
 		split->iova = dma->iova + offset + *size;
 		split->vaddr = dma->vaddr + offset + *size;
 		split->prot = dma->prot;
 		vfio_insert_dma(iommu, split);
-	} else
-		kfree(split);
+	}
 
 	return 0;
 }
#1 lore raw
Quoted 2 lines · Dan Carpenter

If vfio_unmap_unpin() returns an error then we leak "split". I've moved the allocation later in the function to fix this.

Thanks for spotting this and for the patch. The allocation is done early because if we get an allocation failure later, we lose track of iommu mappings, which leads to other leaks. I think it's best to fix the exit path, but we can improve the comment and fix an inconsistent return value at the same time. Thanks,

Alex

vfio/type1: Fix leak on error path

We also don't handle unpinning zero pages as an error on other exits so we can fix that inconsistency by rolling in the next conditional return.

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Alex Williamson <[email protected]>
drivers/vfio/vfio_iommu_type1.c +8 -5
index 98231d1..a9807de 100644
@@ -436,6 +436,12 @@ static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
 	}
 
 	/* Split existing */
+
+	/*
+	 * Allocate our tracking structure early even though it may not
+	 * be used.  An Allocation failure later loses track of pages and
+	 * is more difficult to unwind.
+	 */
 	split = kzalloc(sizeof(*split), GFP_KERNEL);
 	if (!split)
 		return -ENOMEM;

@@ -443,12 +449,9 @@ static int vfio_remove_dma_overlap(struct vfio_iommu *iommu, dma_addr_t start,
 	offset = start - dma->iova;
 
 	ret = vfio_unmap_unpin(iommu, dma, start, size);
-	if (ret)
-		return ret;
-
-	if (!*size) {
+	if (ret || !*size) {
 		kfree(split);
-		return -EINVAL;
+		return ret;
 	}
 
 	tmp = dma->size;