[PATCH 0/4 v2] Implement access checks in iommu page fault paths

[PATCH 0/4 v2] Implement access checks in iommu page fault paths

[Joerg Roedel]

Hi,

here is the second version of the patch-set to implement
proper access checks into the io-page-fault handlers of the
AMD IOMMU and Intel VT-d drivers.

Two additional patches clean up the AMD part a bit further.
Since I can't test this this code myself due to lack of
hardware or software that utilizes it, I'd appreciate some
external testing.

It took me a while to get these out, mostly because I tried
to setup my own HSA test environment to at least test the
AMD changes myself. That failed, so I am sending this out
with another request for testing.

Oded, Jesse, would you two please test these patches and
report back? Thanks a lot!


	Joerg

Changes since v1:

	* Updated the access_error functions based on
	  Linus' feedback
	* Rebased to v4.4-rc1

Joerg Roedel (4):
  iommu/amd: Do proper access checking before calling handle_mm_fault()
  iommu/amd: Correctly set flags for handle_mm_fault call
  iommu/amd: Cleanup error handling in do_fault()
  iommu/vt-d: Do access checks before calling handle_mm_fault()

 drivers/iommu/amd_iommu_v2.c | 54 +++++++++++++++++++++++++++-----------------
 drivers/iommu/intel-svm.c    | 20 ++++++++++++++++
 2 files changed, 53 insertions(+), 21 deletions(-)

-- 
1.9.1

[PATCH 1/4] iommu/amd: Do proper access checking before calling handle_mm_fault()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The handle_mm_fault function expects the caller to do the
access checks. Not doing so and calling the function with
wrong permissions is a bug (catched by a BUG_ON).
So fix this bug by adding proper access checking to the io
page-fault code in the AMD IOMMUv2 driver.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_v2.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index d21d4ed..7caf2fa 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -494,6 +494,22 @@ static void handle_fault_error(struct fault *fault)
 	}
 }
 
+static bool access_error(struct vm_area_struct *vma, struct fault *fault)
+{
+	unsigned long requested = 0;
+
+	if (fault->flags & PPR_FAULT_EXEC)
+		requested |= VM_EXEC;
+
+	if (fault->flags & PPR_FAULT_READ)
+		requested |= VM_READ;
+
+	if (fault->flags & PPR_FAULT_WRITE)
+		requested |= VM_WRITE;
+
+	return (requested & ~vma->vm_flags) != 0;
+}
+
 static void do_fault(struct work_struct *work)
 {
 	struct fault *fault = container_of(work, struct fault, work);
@@ -516,8 +532,8 @@ static void do_fault(struct work_struct *work)
 		goto out;
 	}
 
-	if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE))) {
-		/* handle_mm_fault would BUG_ON() */
+	/* Check if we have the right permissions on the vma */
+	if (access_error(vma, fault)) {
 		up_read(&mm->mmap_sem);
 		handle_fault_error(fault);
 		goto out;
-- 
1.9.1

[PATCH 2/4] iommu/amd: Correctly set flags for handle_mm_fault call

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Instead of just checking for a write access, calculate the
flags that are passed to handle_mm_fault() more precisly and
use the pre-defined macros.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_v2.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index 7caf2fa..a7edbd6 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -513,16 +513,20 @@ static bool access_error(struct vm_area_struct *vma, struct fault *fault)
 static void do_fault(struct work_struct *work)
 {
 	struct fault *fault = container_of(work, struct fault, work);
-	struct mm_struct *mm;
 	struct vm_area_struct *vma;
+	unsigned int flags = 0;
+	struct mm_struct *mm;
 	u64 address;
-	int ret, write;
-
-	write = !!(fault->flags & PPR_FAULT_WRITE);
+	int ret;
 
 	mm = fault->state->mm;
 	address = fault->address;
 
+	if (fault->flags & PPR_FAULT_USER)
+		flags |= FAULT_FLAG_USER;
+	if (fault->flags & PPR_FAULT_WRITE)
+		flags |= FAULT_FLAG_WRITE;
+
 	down_read(&mm->mmap_sem);
 	vma = find_extend_vma(mm, address);
 	if (!vma || address < vma->vm_start) {
@@ -539,7 +543,7 @@ static void do_fault(struct work_struct *work)
 		goto out;
 	}
 
-	ret = handle_mm_fault(mm, vma, address, write);
+	ret = handle_mm_fault(mm, vma, address, flags);
 	if (ret & VM_FAULT_ERROR) {
 		/* failed to service fault */
 		up_read(&mm->mmap_sem);
-- 
1.9.1

[PATCH 3/4] iommu/amd: Cleanup error handling in do_fault()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Get rid of the three error paths that look the same and move
error handling to a single place.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu_v2.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
index a7edbd6..6a28b74 100644
--- a/drivers/iommu/amd_iommu_v2.c
+++ b/drivers/iommu/amd_iommu_v2.c
@@ -514,10 +514,10 @@ static void do_fault(struct work_struct *work)
 {
 	struct fault *fault = container_of(work, struct fault, work);
 	struct vm_area_struct *vma;
+	int ret = VM_FAULT_ERROR;
 	unsigned int flags = 0;
 	struct mm_struct *mm;
 	u64 address;
-	int ret;
 
 	mm = fault->state->mm;
 	address = fault->address;
@@ -529,31 +529,23 @@ static void do_fault(struct work_struct *work)
 
 	down_read(&mm->mmap_sem);
 	vma = find_extend_vma(mm, address);
-	if (!vma || address < vma->vm_start) {
+	if (!vma || address < vma->vm_start)
 		/* failed to get a vma in the right range */
-		up_read(&mm->mmap_sem);
-		handle_fault_error(fault);
 		goto out;
-	}
 
 	/* Check if we have the right permissions on the vma */
-	if (access_error(vma, fault)) {
-		up_read(&mm->mmap_sem);
-		handle_fault_error(fault);
+	if (access_error(vma, fault))
 		goto out;
-	}
 
 	ret = handle_mm_fault(mm, vma, address, flags);
-	if (ret & VM_FAULT_ERROR) {
-		/* failed to service fault */
-		up_read(&mm->mmap_sem);
-		handle_fault_error(fault);
-		goto out;
-	}
 
+out:
 	up_read(&mm->mmap_sem);
 
-out:
+	if (ret & VM_FAULT_ERROR)
+		/* failed to service fault */
+		handle_fault_error(fault);
+
 	finish_pri_tag(fault->dev_state, fault->state, fault->tag);
 
 	put_pasid_state(fault->state);
-- 
1.9.1

[PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Not doing so is a bug and might trigger a BUG_ON in
handle_mm_fault(). So add the proper permission checks
before calling into mm code.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/intel-svm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index c69e3f9..5046483 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -484,6 +484,23 @@ struct page_req_dsc {
 };
 
 #define PRQ_RING_MASK ((0x1000 << PRQ_ORDER) - 0x10)
+
+static bool access_error(struct vm_area_struct *vma, struct page_req_dsc *req)
+{
+	unsigned long requested = 0;
+
+	if (req->exe_req)
+		requested |= VM_EXEC;
+
+	if (req->rd_req)
+		requested |= VM_READ;
+
+	if (req->wr_req)
+		requested |= VM_WRITE;
+
+	return (requested & ~vma->vm_flags) != 0;
+}
+
 static irqreturn_t prq_event_thread(int irq, void *d)
 {
 	struct intel_iommu *iommu = d;
@@ -539,6 +556,9 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		if (!vma || address < vma->vm_start)
 			goto invalid;
 
+		if (access_error(vma, req))
+			goto invalid;
+
 		ret = handle_mm_fault(svm->mm, vma, address,
 				      req->wr_req ? FAULT_FLAG_WRITE : 0);
 		if (ret & VM_FAULT_ERROR)
-- 
1.9.1

Re: [PATCH 0/4 v2] Implement access checks in iommu page fault paths

[Oded Gabbay]

On Tue, Nov 17, 2015 at 5:11 PM, Joerg Roedel <joro@8bytes.org> wrote:
> Hi,
>
> here is the second version of the patch-set to implement
> proper access checks into the io-page-fault handlers of the
> AMD IOMMU and Intel VT-d drivers.
>
> Two additional patches clean up the AMD part a bit further.
> Since I can't test this this code myself due to lack of
> hardware or software that utilizes it, I'd appreciate some
> external testing.
>
> It took me a while to get these out, mostly because I tried
> to setup my own HSA test environment to at least test the
> AMD changes myself. That failed, so I am sending this out
> with another request for testing.
>
> Oded, Jesse, would you two please test these patches and
> report back? Thanks a lot!
>
>
>         Joerg
>
> Changes since v1:
>
>         * Updated the access_error functions based on
>           Linus' feedback
>         * Rebased to v4.4-rc1
>
> Joerg Roedel (4):
>   iommu/amd: Do proper access checking before calling handle_mm_fault()
>   iommu/amd: Correctly set flags for handle_mm_fault call
>   iommu/amd: Cleanup error handling in do_fault()
>   iommu/vt-d: Do access checks before calling handle_mm_fault()
>
>  drivers/iommu/amd_iommu_v2.c | 54 +++++++++++++++++++++++++++-----------------
>  drivers/iommu/intel-svm.c    | 20 ++++++++++++++++
>  2 files changed, 53 insertions(+), 21 deletions(-)
>
> --
> 1.9.1
>

Hi Joerg,

I have very limited testing capabilities since I left AMD - I only
have Kaveri (which is kind of obsolete) and I only have a very simple
test utility.

>From what I could check, I didn't see any problems, but this is really
such a very basic check that I don't even want to presume to give a
Tested-by tag. Sorry.

I suggest you talk to AMD people, as they have access to Carrizo H/W
and S/W testing framework.

        Oded

Re: [PATCH 0/4 v2] Implement access checks in iommu page fault paths

[Jesse Barnes]

On 11/17/2015 07:11 AM, Joerg Roedel wrote:
> Hi,
> 
> here is the second version of the patch-set to implement
> proper access checks into the io-page-fault handlers of the
> AMD IOMMU and Intel VT-d drivers.
> 
> Two additional patches clean up the AMD part a bit further.
> Since I can't test this this code myself due to lack of
> hardware or software that utilizes it, I'd appreciate some
> external testing.
> 
> It took me a while to get these out, mostly because I tried
> to setup my own HSA test environment to at least test the
> AMD changes myself. That failed, so I am sending this out
> with another request for testing.
> 
> Oded, Jesse, would you two please test these patches and
> report back? Thanks a lot!

I might be able to find time later this week or next; I have to update
my tree to David's latest bits and rebase my i915 code on top along with
these patches.  Then I can run some of my tests and expand them to try
r/w tests w/o mapping the buffer with the right access mode.

Jesse

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On Tue, 2015-11-17 at 16:11 +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Not doing so is a bug and might trigger a BUG_ON in
> handle_mm_fault(). So add the proper permission checks
> before calling into mm code.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Acked-By: David Woodhouse <David.Woodhouse@intel.com>

As noted, I can't test it right now until I've replaced my test
hardware. Certainly looks better than the last version though :)

Thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


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

Re: [PATCH 0/4 v2] Implement access checks in iommu page fault paths

[Joerg Roedel]

Hey Jesse,

On Wed, Nov 18, 2015 at 08:13:56AM -0800, Jesse Barnes wrote:
> 
> I might be able to find time later this week or next; I have to update
> my tree to David's latest bits and rebase my i915 code on top along with
> these patches.  Then I can run some of my tests and expand them to try
> r/w tests w/o mapping the buffer with the right access mode.

Have you found time to run some small tests on this?


Thanks,

	Joerg

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Joerg Roedel]

On Fri, Nov 20, 2015 at 03:35:53PM +0000, David Woodhouse wrote:
> Acked-By: David Woodhouse <David.Woodhouse@intel.com>
> 
> As noted, I can't test it right now until I've replaced my test
> hardware. Certainly looks better than the last version though :)

Okay, thanks. I applied the series to the iommu tree. Patch 1 and 4 as
fixes, the others the the x86/amd branch.

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Joerg Roedel]

On Tue, Nov 10, 2015 at 09:43:03AM -0800, Linus Torvalds wrote:
> On Tue, Nov 10, 2015 at 5:26 AM, Joerg Roedel <joro@8bytes.org> wrote:
> > +
> > +static bool access_error(struct vm_area_struct *vma, struct page_req_dsc *req)
> > +{
> > +       return !((req->rd_req  && (vma->vm_flags & VM_READ))  ||
> > +                (req->wr_req  && (vma->vm_flags & VM_WRITE)) ||
> > +                (req->exe_req && (vma->vm_flags & VM_EXEC)));
> > +}
> 
> This seems odd.
> 
> Shouldn't it be
> 
>     return ((req->rd_req  && !(vma->vm_flags & VM_READ))  ||
>             (req->wr_req  && !(vma->vm_flags & VM_WRITE)) ||
>             (req->exe_req && !(vma->vm_flags & VM_EXEC)));
> 
> instead?

Yes,  thats better, it solves the multiple-bits-set problem too, which
David mentioned.

> Of course, if you just used the VM_xyz flags internally itself, this
> would all be easier, and you'd end up with something like
> 
>     /* Do we have requested bits that aren't in the allowed set? */
>     return (requested & ~vma->vm_flags) != 0;
> 
> instead..

But this is probably the best solution, some architecture page-fault
handlers do something similar. I'll update the patch and use this for
the AMD part too.


Thanks,

	Joerg

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Linus Torvalds]

On Tue, Nov 10, 2015 at 5:26 AM, Joerg Roedel <joro@8bytes.org> wrote:
> +
> +static bool access_error(struct vm_area_struct *vma, struct page_req_dsc *req)
> +{
> +       return !((req->rd_req  && (vma->vm_flags & VM_READ))  ||
> +                (req->wr_req  && (vma->vm_flags & VM_WRITE)) ||
> +                (req->exe_req && (vma->vm_flags & VM_EXEC)));
> +}

This seems odd.

Shouldn't it be

    return ((req->rd_req  && !(vma->vm_flags & VM_READ))  ||
            (req->wr_req  && !(vma->vm_flags & VM_WRITE)) ||
            (req->exe_req && !(vma->vm_flags & VM_EXEC)));

instead?

Of course, if you just used the VM_xyz flags internally itself, this
would all be easier, and you'd end up with something like

    /* Do we have requested bits that aren't in the allowed set? */
    return (requested & ~vma->vm_flags) != 0;

instead..

              Linus

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Joerg Roedel]

On Tue, Nov 10, 2015 at 02:45:56PM +0000, David Woodhouse wrote:
> On Tue, 2015-11-10 at 14:26 +0100, Joerg Roedel wrote:
> > +static bool access_error(struct vm_area_struct *vma, struct
> > page_req_dsc *req)
> > +{
> > +	return !((req->rd_req  && (vma->vm_flags & VM_READ))  ||
> > +		 (req->wr_req  && (vma->vm_flags & VM_WRITE)) ||
> > +		 (req->exe_req && (vma->vm_flags & VM_EXEC)));
> > +}
> > +
> 
> This is a TLB fill request from the device — can it not be asking for
> *all* of read, write and exec privs? And you allow it to succeed if any
> *one* of the permissions that it asks for is available?
> 
> Even if we don't see read+write in the same request, the VT-d spec does
> seem quite clear that we *will* see read+exec (§7.5.1.1 p7-17 of v2.3:
> 
> • Execute Requested: If the PASID Present, Read Requested and Execute
>   Requested fields are all 1, the request-with-PASID that encountered 
>   the recoverable fault that resulted in this page request, requires 
>   execute access to the page.

Okay, thanks for the clarification. The code above assumes that only one
of RWX is set. I'll update the patch to correctly check when multiple
bits are set.


	Joerg

Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1526 bytes --]

On Tue, 2015-11-10 at 14:26 +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Not doing so is a bug and might trigger a BUG_ON in
> handle_mm_fault(). So add the proper permission checks
> before calling into mm code.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>


> +static bool access_error(struct vm_area_struct *vma, struct
> page_req_dsc *req)
> +{
> +	return !((req->rd_req  && (vma->vm_flags & VM_READ))  ||
> +		 (req->wr_req  && (vma->vm_flags & VM_WRITE)) ||
> +		 (req->exe_req && (vma->vm_flags & VM_EXEC)));
> +}
> +

This is a TLB fill request from the device — can it not be asking for
*all* of read, write and exec privs? And you allow it to succeed if any
*one* of the permissions that it asks for is available?

Even if we don't see read+write in the same request, the VT-d spec does
seem quite clear that we *will* see read+exec (§7.5.1.1 p7-17 of v2.3:

• Execute Requested: If the PASID Present, Read Requested and Execute
  Requested fields are all 1, the request-with-PASID that encountered 
  the recoverable fault that resulted in this page request, requires 
  execute access to the page.

Also, I'm afraid my Skylake box blew up the day I sent the pull request
to Linus so I'm unable to test until I've sorted out a replacement.
Jesse should be able to though...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


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

[PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Not doing so is a bug and might trigger a BUG_ON in
handle_mm_fault(). So add the proper permission checks
before calling into mm code.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/intel-svm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index c69e3f9..4ebf097 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -484,6 +484,14 @@ struct page_req_dsc {
 };
 
 #define PRQ_RING_MASK ((0x1000 << PRQ_ORDER) - 0x10)
+
+static bool access_error(struct vm_area_struct *vma, struct page_req_dsc *req)
+{
+	return !((req->rd_req  && (vma->vm_flags & VM_READ))  ||
+		 (req->wr_req  && (vma->vm_flags & VM_WRITE)) ||
+		 (req->exe_req && (vma->vm_flags & VM_EXEC)));
+}
+
 static irqreturn_t prq_event_thread(int irq, void *d)
 {
 	struct intel_iommu *iommu = d;
@@ -539,6 +547,9 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		if (!vma || address < vma->vm_start)
 			goto invalid;
 
+		if (access_error(vma, req))
+			goto invalid;
+
 		ret = handle_mm_fault(svm->mm, vma, address,
 				      req->wr_req ? FAULT_FLAG_WRITE : 0);
 		if (ret & VM_FAULT_ERROR)
-- 
1.9.1