Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1266486 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] Implement access checks in iommu page fault paths Joerg Roedel <joro@8bytes.org> - 2015-11-10 14:30 +0100
    [PATCH 2/4] iommu/amd: Correctly set flags for handle_mm_fault call Joerg Roedel <joro@8bytes.org> - 2015-11-10 14:30 +0100
    [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault() Joerg Roedel <joro@8bytes.org> - 2015-11-10 14:30 +0100
      Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling  handle_mm_fault() David Woodhouse <dwmw2@infradead.org> - 2015-11-10 15:50 +0100
        Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling  handle_mm_fault() Joerg Roedel <jroedel@suse.de> - 2015-11-10 16:00 +0100
      Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling handle_mm_fault() Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-10 18:50 +0100
        Re: [PATCH 4/4] iommu/vt-d: Do access checks before calling  handle_mm_fault() Joerg Roedel <joro@8bytes.org> - 2015-11-10 19:10 +0100
    [PATCH 3/4] iommu/amd: Cleanup error handling in do_fault() Joerg Roedel <joro@8bytes.org> - 2015-11-10 14:30 +0100
    [PATCH 1/4] iommu/amd: Do proper access checking before calling handle_mm_fault() Joerg Roedel <joro@8bytes.org> - 2015-11-10 14:30 +0100

#1266486 — [PATCH 0/4] Implement access checks in iommu page fault paths

Hi,

here is a patch-set that 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.

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


	Joerg


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    | 11 +++++++++
 2 files changed, 44 insertions(+), 21 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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 612f7c8..afa8f9c 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 afa8f9c..cd1a222 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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..612f7c8 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 int allowed = 0;
+
+	if (fault->flags & PPR_FAULT_EXEC)
+		allowed = vma->vm_flags & VM_EXEC;
+
+	if (fault->flags & PPR_FAULT_READ)
+		allowed = vma->vm_flags & VM_READ;
+
+	if (fault->flags & PPR_FAULT_WRITE)
+		allowed = vma->vm_flags & VM_WRITE;
+
+	return allowed ? false : true;
+}
+
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web