| Message ID | 20200109141152.975687-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Validation rotated vma bounds are within the object | expand |
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
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
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.
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;
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(-)