drm/i915/gt: Validation rotated vma bounds are within the object
diff mbox series

Message ID 20200109141152.975687-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gt: Validation rotated vma bounds are within the object
Related show

Commit Message

Chris Wilson Jan. 9, 2020, 2:11 p.m. UTC
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.

Closes: https://gitlab.freedesktop.org/drm/intel/issues/951
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matthew Auld <matthew.auld@intel.com
---
 drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Ville Syrjälä Jan. 9, 2020, 2:52 p.m. UTC | #1
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?

> 
> Closes: https://gitlab.freedesktop.org/drm/intel/issues/951
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@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ä Jan. 9, 2020, 6:37 p.m. UTC | #2
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@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matthew Auld <matthew.auld@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ä Jan. 9, 2020, 7:01 p.m. UTC | #3
On Thu, Jan 09, 2020 at 08:37:09PM +0200, Ville Syrjälä wrote:
> 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...

That may well be it. Looks like we may not end up calling
intel_plane_copy_uapi_to_hw_state() for old slave planes at all,
thus leaving a stale plane_state->hw.fb pointer behind.

Patch
diff mbox series

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;