[Intel-gfx] [PATCH] drm/i915/gt: Validation rotated vma bounds are within the object

[Ville Syrjälä]

On Thu, Jan 09, 2020 at 04:52:41PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 09, 2020 at 02:11:52PM +0000, Chris Wilson wrote:
> > Quite understandably, we bug out when asked to find a page that doesn't
> > belong to the object. However, we should report the error back to the
> > user long before we attempt the out-of-bound access! In this case, it is
> > insufficient validation on the rotated vma, with the simplest/cheapest
> > point for us to insert a bound check when we are computing the rotated
> > page lookups.
> > 
> > Similarly, it might be wise to see if we can validate the user input
> > upon creating the rotated framebuffer.
> 
> We do. Did someone break it?

One theory on how this could happens is that we are using a stale gtt
view here. But AFAICS the only way that could happen is that we take
a shortcut out from the plane check somewhere before populating
plane_state->gtt_view afresh, after using a rotated fb previously so
that plane_state->gtt_view has been populated with a rotated view.

The first such path I see is:
intel_plane_atomic_check_with_state()
{
...
	if (!new_plane_state->hw.crtc && !old_plane_state->hw.crtc)
		return 0;

but that should also imply new_plane_state->hw.fb==NULL and so we
should not end up pinning the fb.

The second path is:
intel_plane_compute_gtt()
{
	const struct intel_framebuffer *fb =
	        to_intel_framebuffer(plane_state->hw.fb);

	if (!fb)
		return 0;

and so we won't have a new fb there either and so shouldn't try
to pin it.

So can't see how that could happen from these normal paths. Which
leads me to wonder if this might have something to do with nv12
slave planes...

> 
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/issues/951
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com
> > ---
> >  drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 ++++++++++++++++++---
> >  1 file changed, 18 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > index 99189cdba8a9..59a60968a6da 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > @@ -1283,6 +1283,7 @@ static noinline struct sg_table *
> >  intel_rotate_pages(struct intel_rotation_info *rot_info,
> >  		   struct drm_i915_gem_object *obj)
> >  {
> > +	const unsigned long npages = obj->base.size >> PAGE_SHIFT;
> >  	unsigned int size = intel_rotation_info_size(rot_info);
> >  	struct sg_table *st;
> >  	struct scatterlist *sg;
> > @@ -1302,9 +1303,23 @@ intel_rotate_pages(struct intel_rotation_info *rot_info,
> >  	sg = st->sgl;
> >  
> >  	for (i = 0 ; i < ARRAY_SIZE(rot_info->plane); i++) {
> > -		sg = rotate_pages(obj, rot_info->plane[i].offset,
> > -				  rot_info->plane[i].width, rot_info->plane[i].height,
> > -				  rot_info->plane[i].stride, st, sg);
> > +		const struct intel_remapped_plane_info *plane =
> > +			&rot_info->plane[i];
> > +		unsigned long last;
> > +
> > +		last = plane->offset;
> > +		last += (plane->height - 1) * plane->stride;
> > +		last += plane->width - 1;
> > +		if (last >= npages) {
> > +			ret = -EINVAL;
> > +			goto err_sg_alloc;
> > +		}
> > +
> > +		sg = rotate_pages(obj,
> > +				  plane->offset,
> > +				  plane->width, plane->height,
> > +				  plane->stride,
> > +				  st, sg);
> >  	}
> >  
> >  	return st;
> > -- 
> > 2.25.0.rc2
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel