re: iommu/vt-d: Implement page request handling

re: iommu/vt-d: Implement page request handling

[Dan Carpenter]

Hello David Woodhouse,

This is a semi-automatic email about new static checker warnings.

The patch 0b9252a34858: "iommu/vt-d: Implement page request handling" 
from Oct 8, 2015, leads to the following Smatch complaint:

drivers/iommu/intel-svm.c:452 prq_event_thread()
	 error: we previously assumed 'svm' could be null (see line 412)

drivers/iommu/intel-svm.c
   411				svm = idr_find(&iommu->pasid_idr, req->pasid);
   412				if (!svm) {

The patch adds a NULL check here.

   413					pr_err("%s: Page request for invalid PASID %d: %08llx %08llx\n",
   414					       iommu->name, req->pasid, ((unsigned long long *)req)[0],
   415					       ((unsigned long long *)req)[1]);
   416					mutex_unlock(&pasid_mutex);
   417					goto inval_req;
   418				}
   419				/* Strictly speaking, we shouldn't need this. It is forbidden
   420				   to unbind the PASID while there may still be requests in
   421				   flight. But let's do it anyway. */
   422				kref_get(&svm->kref);
   423				mutex_unlock(&pasid_mutex);
   424			}
   425	
   426			result = QI_RESP_FAILURE;
   427			down_read(&svm->mm->mmap_sem);
   428			vma = find_extend_vma(svm->mm, address);
   429			if (!vma || address < vma->vm_start)
   430				goto hard_fault;
   431	
   432			ret = handle_mm_fault(svm->mm, vma, address,
   433					      req->wr_req ? FAULT_FLAG_WRITE : 0);
   434			if (ret & VM_FAULT_ERROR)
   435				goto hard_fault;
   436	
   437			result = QI_RESP_SUCCESS;
   438		hard_fault:
   439			up_read(&svm->mm->mmap_sem);
   440		inval_req:
   441			/* Accounting for major/minor faults? */
   442	
   443			if (req->lpig) {

Perhaps this check ensures that "svm" is not NULL but I don't know the
code well enough to say.

   444				/* Page Group Response */
   445				resp.low = QI_PGRP_PASID(req->pasid) |
   446					QI_PGRP_DID((req->bus << 8) | req->devfn) |
   447					QI_PGRP_PASID_P(req->pasid_present) |
   448					QI_PGRP_RESP_TYPE;
   449				resp.high = QI_PGRP_IDX(req->prg_index) |
   450					QI_PGRP_PRIV(req->private) | QI_PGRP_RESP_CODE(result);
   451	
   452				qi_submit_sync(&resp, svm->iommu);
                                                      ^^^^^^^^^^
The patch introduces a NULL dereference here.

   453			} else if (req->srr) {
   454				/* Page Stream Response */

regards,
dan carpenter

Re: iommu/vt-d: Implement page request handling

[Woodhouse, David]

[-- Attachment #1.1: Type: text/plain, Size: 2215 bytes --]

On Thu, 2015-10-15 at 21:25 +0300, Dan Carpenter wrote:
> 
> The patch 0b9252a34858: "iommu/vt-d: Implement page request handling" 
> from Oct 8, 2015, leads to the following Smatch complaint:
> 
> drivers/iommu/intel-svm.c:452 prq_event_thread()
>          error: we previously assumed 'svm' could be null (see line 412)

Thanks. I'm going to fix it thus:

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 817be76..b7e923a 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -510,7 +510,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 				pr_err("%s: Page request for invalid PASID %d: %08llx %08llx\n",
 				       iommu->name, req->pasid, ((unsigned long long *)req)[0],
 				       ((unsigned long long *)req)[1]);
-				goto bad_req;
+				goto no_pasid;
 			}
 		}
 
@@ -552,7 +552,11 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 				(req->wr_req << 1) | (req->exe_req);
 			sdev->ops->fault_cb(sdev->dev, req->pasid, req->addr, req->private, rwxp, result);
 		}
-
+		/* We get here in the error case where the PASID lookup failed,
+		   and these can be NULL. Do not use them below this point! */
+		sdev = NULL;
+		svm = NULL;
+	no_pasid:
 		if (req->lpig) {
 			/* Page Group Response */
 			resp.low = QI_PGRP_PASID(req->pasid) |
@@ -562,7 +566,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			resp.high = QI_PGRP_IDX(req->prg_index) |
 				QI_PGRP_PRIV(req->private) | QI_PGRP_RESP_CODE(result);
 
-			qi_submit_sync(&resp, svm->iommu);
+			qi_submit_sync(&resp, iommu);
 		} else if (req->srr) {
 			/* Page Stream Response */
 			resp.low = QI_PSTRM_IDX(req->prg_index) |
@@ -571,7 +575,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			resp.high = QI_PSTRM_ADDR(address) | QI_PSTRM_DEVFN(req->devfn) |
 				QI_PSTRM_RESP_CODE(result);
 
-			qi_submit_sync(&resp, svm->iommu);
+			qi_submit_sync(&resp, iommu);
 		}
 
 		head = (head + sizeof(*req)) & PRQ_RING_MASK;
-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation


[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3437 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]