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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel