[PATCH] drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

[PATCH] drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

[Chris Wilson]

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(-)

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,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

[Matthew Auld]

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>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

[Patchwork]

== Series Details ==

Series: drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view
URL   : https://patchwork.freedesktop.org/series/18414/
State : success

== Summary ==

Series 18414v1 drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view
https://patchwork.freedesktop.org/api/1.0/series/18414/revisions/1/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

5c31c0247c4b947cd0af336ddfc5798c74402863 drm-tip: 2017y-01m-23d-12h-27m-17s UTC integration manifest
57701a9 drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3584/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Pevent copying uninitialised garbage into vma->ggtt_view

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx