| Message ID | 20170123145245.3972-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 January 2017 at 14:52, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Since tweaking i915_vma_compare() we allowed constructors to skip > clearing the ggtt_view believing that we didn't access the unused > members. That, as it turns out, was not entirely true. In particular, > i915_gem_fault() uses > > ret = remap_io_mapping(area, > area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT), > (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT, > min_t(u64, vma->size, area->vm_end - area->vm_start), > &ggtt->mappable); > > i.e. the ggtt_view.partial for both normal and partial views. If we > allowed garbage into the normal vma->ggtt_view and then try userspace > tried to mmap it, we could explode in an unobvious fashion. > > Fixes: 7b92c047bae2 ("drm/i915: Eliminate superfluous i915_ggtt_view_rotated") > Fixes: 3bf4d5751943 ("drm/i915: Stop clearing i915_ggtt_view") > Reported-by: Matthew Auld <matthew.william.auld@gmail.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matthew Auld <matthew.william.auld@gmail.com> Tested-by: Matthew Auld <matthew.auld@intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
On Mon, Jan 23, 2017 at 03:01:20PM +0000, Matthew Auld wrote: > On 23 January 2017 at 14:52, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Since tweaking i915_vma_compare() we allowed constructors to skip > > clearing the ggtt_view believing that we didn't access the unused > > members. That, as it turns out, was not entirely true. In particular, > > i915_gem_fault() uses > > > > ret = remap_io_mapping(area, > > area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT), > > (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT, > > min_t(u64, vma->size, area->vm_end - area->vm_start), > > &ggtt->mappable); > > > > i.e. the ggtt_view.partial for both normal and partial views. If we > > allowed garbage into the normal vma->ggtt_view and then try userspace > > tried to mmap it, we could explode in an unobvious fashion. > > > > Fixes: 7b92c047bae2 ("drm/i915: Eliminate superfluous i915_ggtt_view_rotated") > > Fixes: 3bf4d5751943 ("drm/i915: Stop clearing i915_ggtt_view") > > Reported-by: Matthew Auld <matthew.william.auld@gmail.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > Tested-by: Matthew Auld <matthew.auld@intel.com> > Reviewed-by: Matthew Auld <matthew.auld@intel.com> Thanks for quickly finding this. -Chris
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 307b22ae7791..155906e84812 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -91,7 +91,7 @@ vma_create(struct drm_i915_gem_object *obj, vma->size = obj->base.size; vma->display_alignment = I915_GTT_MIN_ALIGNMENT; - if (view) { + if (view && view->type != I915_GGTT_VIEW_NORMAL) { vma->ggtt_view = *view; if (view->type == I915_GGTT_VIEW_PARTIAL) { GEM_BUG_ON(range_overflows_t(u64,
Since tweaking i915_vma_compare() we allowed constructors to skip clearing the ggtt_view believing that we didn't access the unused members. That, as it turns out, was not entirely true. In particular, i915_gem_fault() uses ret = remap_io_mapping(area, area->vm_start + (vma->ggtt_view.partial.offset << PAGE_SHIFT), (ggtt->mappable_base + vma->node.start) >> PAGE_SHIFT, min_t(u64, vma->size, area->vm_end - area->vm_start), &ggtt->mappable); i.e. the ggtt_view.partial for both normal and partial views. If we allowed garbage into the normal vma->ggtt_view and then try userspace tried to mmap it, we could explode in an unobvious fashion. Fixes: 7b92c047bae2 ("drm/i915: Eliminate superfluous i915_ggtt_view_rotated") Fixes: 3bf4d5751943 ("drm/i915: Stop clearing i915_ggtt_view") Reported-by: Matthew Auld <matthew.william.auld@gmail.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matthew Auld <matthew.william.auld@gmail.com> --- drivers/gpu/drm/i915/i915_vma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)