| Message ID | 20180614115942.27773-3-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Thursday, June 14, 2018 5:00 AM > To: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > <jon.bloomfield@intel.com>; Joonas Lahtinen > <joonas.lahtinen@linux.intel.com>; Matthew Auld > <matthew.william.auld@gmail.com> > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a > GGTT mmap > > If the user has created a read-only object, they should not be allowed > to circumvent the write protection by using a GGTT mmapping. Deny it. > > Also most machines do not support read-only GGTT PTEs, so again we have > to reject attempted writes. Fortunately, this is known a priori, so we > can at least reject in the call to create the mmap with backup in the > fault handler. This is a little draconian as we could blatantly ignore > the write protection on the pages, but it is far simply to keep the > readonly object pure. (It is easier to lift a restriction than to impose > it later!) Are you sure this is necessary? I assumed you would just create a ro IA mapping to the page, irrespective of the ability of ggtt. It feels wrong to disallow mapping a read-only object to the CPU as read-only. With ppgtt the presence of an unprotected mapping in the ggtt should be immune from tampering in the GT, so only the cpu mapping should really matter. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matthew Auld <matthew.william.auld@gmail.com> > ---
Quoting Bloomfield, Jon (2018-06-14 15:53:13) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Thursday, June 14, 2018 5:00 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > > <jon.bloomfield@intel.com>; Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com>; Matthew Auld > > <matthew.william.auld@gmail.com> > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via a > > GGTT mmap > > > > If the user has created a read-only object, they should not be allowed > > to circumvent the write protection by using a GGTT mmapping. Deny it. > > > > Also most machines do not support read-only GGTT PTEs, so again we have > > to reject attempted writes. Fortunately, this is known a priori, so we > > can at least reject in the call to create the mmap with backup in the > > fault handler. This is a little draconian as we could blatantly ignore > > the write protection on the pages, but it is far simply to keep the > > readonly object pure. (It is easier to lift a restriction than to impose > > it later!) > Are you sure this is necessary? I assumed you would just create a ro IA > mapping to the page, irrespective of the ability of ggtt. You are thinking of the CPU mmap? The GTT mmap offers a linear view of the tiled object. It would be very wrong for us to bypass the PROT_READ protection of a user page by accessing it via the GTT. > It feels wrong to > disallow mapping a read-only object to the CPU as read-only. With ppgtt > the presence of an unprotected mapping in the ggtt should be immune > from tampering in the GT, so only the cpu mapping should really matter. And the CPU mapping has its protection bits on the IA PTE. -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Thursday, June 14, 2018 8:00 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > <matthew.william.auld@gmail.com> > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via > a GGTT mmap > > Quoting Bloomfield, Jon (2018-06-14 15:53:13) > > > -----Original Message----- > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > Sent: Thursday, June 14, 2018 5:00 AM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > > > <jon.bloomfield@intel.com>; Joonas Lahtinen > > > <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > <matthew.william.auld@gmail.com> > > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via > a > > > GGTT mmap > > > > > > If the user has created a read-only object, they should not be allowed > > > to circumvent the write protection by using a GGTT mmapping. Deny it. > > > > > > Also most machines do not support read-only GGTT PTEs, so again we > have > > > to reject attempted writes. Fortunately, this is known a priori, so we > > > can at least reject in the call to create the mmap with backup in the > > > fault handler. This is a little draconian as we could blatantly ignore > > > the write protection on the pages, but it is far simply to keep the > > > readonly object pure. (It is easier to lift a restriction than to impose > > > it later!) > > Are you sure this is necessary? I assumed you would just create a ro IA > > mapping to the page, irrespective of the ability of ggtt. > > You are thinking of the CPU mmap? The GTT mmap offers a linear view of > the tiled object. It would be very wrong for us to bypass the PROT_READ > protection of a user page by accessing it via the GTT. No, I was thinking of gtt mmap. That requires both GTT and IA PTE mappings right? One to map the object into the GTT, and then a second to point the IA at the aperture. Why wouldn't marking the IA mapping RO protect the object if the GT cannot reach the GTT mapping (from user batches). > > > It feels wrong to > > disallow mapping a read-only object to the CPU as read-only. With ppgtt > > the presence of an unprotected mapping in the ggtt should be immune > > from tampering in the GT, so only the cpu mapping should really matter. > > And the CPU mapping has its protection bits on the IA PTE. > -Chris
Quoting Bloomfield, Jon (2018-06-14 16:06:40) > > -----Original Message----- > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Sent: Thursday, June 14, 2018 8:00 AM > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > <matthew.william.auld@gmail.com> > > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via > > a GGTT mmap > > > > Quoting Bloomfield, Jon (2018-06-14 15:53:13) > > > > -----Original Message----- > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Sent: Thursday, June 14, 2018 5:00 AM > > > > To: intel-gfx@lists.freedesktop.org > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > > > > <jon.bloomfield@intel.com>; Joonas Lahtinen > > > > <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > > <matthew.william.auld@gmail.com> > > > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via > > a > > > > GGTT mmap > > > > > > > > If the user has created a read-only object, they should not be allowed > > > > to circumvent the write protection by using a GGTT mmapping. Deny it. > > > > > > > > Also most machines do not support read-only GGTT PTEs, so again we > > have > > > > to reject attempted writes. Fortunately, this is known a priori, so we > > > > can at least reject in the call to create the mmap with backup in the > > > > fault handler. This is a little draconian as we could blatantly ignore > > > > the write protection on the pages, but it is far simply to keep the > > > > readonly object pure. (It is easier to lift a restriction than to impose > > > > it later!) > > > Are you sure this is necessary? I assumed you would just create a ro IA > > > mapping to the page, irrespective of the ability of ggtt. > > > > You are thinking of the CPU mmap? The GTT mmap offers a linear view of > > the tiled object. It would be very wrong for us to bypass the PROT_READ > > protection of a user page by accessing it via the GTT. > No, I was thinking of gtt mmap. That requires both GTT and IA PTE mappings > right? One to map the object into the GTT, and then a second to point the > IA at the aperture. Why wouldn't marking the IA mapping RO protect the > object if the GT cannot reach the GTT mapping (from user batches). Hmm. I keep forgetting that we can get at the vma from mmap(), because that's hidden away elsewhere and only see i915_gem_fault() on a daily basis. Hmm, is legal to read a PROT_READ-only vma is PROT_WRITE is requested, or are meant to return -EINVAL? -Chris
> -----Original Message----- > From: Chris Wilson <chris@chris-wilson.co.uk> > Sent: Thursday, June 14, 2018 8:22 AM > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > <matthew.william.auld@gmail.com> > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only object via > a GGTT mmap > > Quoting Bloomfield, Jon (2018-06-14 16:06:40) > > > -----Original Message----- > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > Sent: Thursday, June 14, 2018 8:00 AM > > > To: Bloomfield, Jon <jon.bloomfield@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > <matthew.william.auld@gmail.com> > > > Subject: RE: [PATCH 3/5] drm/i915: Prevent writing into a read-only > object via > > > a GGTT mmap > > > > > > Quoting Bloomfield, Jon (2018-06-14 15:53:13) > > > > > -----Original Message----- > > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > > Sent: Thursday, June 14, 2018 5:00 AM > > > > > To: intel-gfx@lists.freedesktop.org > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>; Bloomfield, Jon > > > > > <jon.bloomfield@intel.com>; Joonas Lahtinen > > > > > <joonas.lahtinen@linux.intel.com>; Matthew Auld > > > > > <matthew.william.auld@gmail.com> > > > > > Subject: [PATCH 3/5] drm/i915: Prevent writing into a read-only > object via > > > a > > > > > GGTT mmap > > > > > > > > > > If the user has created a read-only object, they should not be allowed > > > > > to circumvent the write protection by using a GGTT mmapping. Deny > it. > > > > > > > > > > Also most machines do not support read-only GGTT PTEs, so again we > > > have > > > > > to reject attempted writes. Fortunately, this is known a priori, so we > > > > > can at least reject in the call to create the mmap with backup in the > > > > > fault handler. This is a little draconian as we could blatantly ignore > > > > > the write protection on the pages, but it is far simply to keep the > > > > > readonly object pure. (It is easier to lift a restriction than to impose > > > > > it later!) > > > > Are you sure this is necessary? I assumed you would just create a ro IA > > > > mapping to the page, irrespective of the ability of ggtt. > > > > > > You are thinking of the CPU mmap? The GTT mmap offers a linear view of > > > the tiled object. It would be very wrong for us to bypass the PROT_READ > > > protection of a user page by accessing it via the GTT. > > No, I was thinking of gtt mmap. That requires both GTT and IA PTE > mappings > > right? One to map the object into the GTT, and then a second to point the > > IA at the aperture. Why wouldn't marking the IA mapping RO protect the > > object if the GT cannot reach the GTT mapping (from user batches). > > Hmm. I keep forgetting that we can get at the vma from mmap(), because > that's hidden away elsewhere and only see i915_gem_fault() on a daily > basis. Hmm, is legal to read a PROT_READ-only vma is PROT_WRITE is > requested, or are meant to return -EINVAL? > -Chris That's a trickier question :-) My instinct in -EINVAL if the user specifies PROT_WRITE, but allowed if they only PROT_READ, and ppgtt is enabled (including aliased) so that userspace can't see the gtt mapping from the GT.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8dd4d35655af..d5eb73f7a90a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2009,6 +2009,10 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf) unsigned int flags; int ret; + /* Sanity check that we allow writing into this object */ + if (obj->gt_ro && (write || !ggtt->vm.has_read_only)) + return VM_FAULT_SIGBUS; + /* We don't use vmf->pgoff since that has the fake offset */ page_offset = (vmf->address - area->vm_start) >> PAGE_SHIFT; @@ -2288,10 +2292,17 @@ i915_gem_mmap_gtt(struct drm_file *file, if (!obj) return -ENOENT; + /* If we will not be able to create the GGTT vma, reject it early. */ + if (obj->gt_ro && !to_i915(dev)->ggtt.vm.has_read_only) { + ret = -ENODEV; + goto out; + } + ret = i915_gem_object_create_mmap_offset(obj); if (ret == 0) *offset = drm_vma_node_offset_addr(&obj->base.vma_node); +out: i915_gem_object_put(obj); return ret; }
If the user has created a read-only object, they should not be allowed to circumvent the write protection by using a GGTT mmapping. Deny it. Also most machines do not support read-only GGTT PTEs, so again we have to reject attempted writes. Fortunately, this is known a priori, so we can at least reject in the call to create the mmap with backup in the fault handler. This is a little draconian as we could blatantly ignore the write protection on the pages, but it is far simply to keep the readonly object pure. (It is easier to lift a restriction than to impose it later!) Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jon Bloomfield <jon.bloomfield@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matthew Auld <matthew.william.auld@gmail.com> --- drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++ 1 file changed, 11 insertions(+)