Re: [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	Michel Thierry <michel.thierry@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: prevent out of range pt in the PDE macros (take 3)
Date: Fri, 2 Oct 2015 12:52:09 +0200	[thread overview]
Message-ID: <20151002105209.GI3383@phenom.ffwll.local> (raw)
In-Reply-To: <20151002085808.GP9929@nuc-i3427.alporthouse.com>

On Fri, Oct 02, 2015 at 09:58:08AM +0100, Chris Wilson wrote:
> On Fri, Oct 02, 2015 at 09:58:05AM +0200, Daniel Vetter wrote:
> > On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> > > We tried to fix this in commit fdc454c1484a ("drm/i915: Prevent out of
> > > range pt in gen6_for_each_pde").
> > > 
> > > But the static analyzer still complains that, just before we break due
> > > to "iter < I915_PDES", we do "pt = (pd)->page_table[iter]" with an
> > > iter value that is bigger than I915_PDES. Of course, this isn't really
> > > a problem since no one uses pt outside the macro. Still, every single
> > > new usage of the macro will create a new issue for us to mark as a
> > > false positive.
> > > 
> > > Also, Paulo re-started the discussion a while ago [1], but didn't end up
> > > implemented.
> > > 
> > > In order to "solve" this "problem", this patch takes the ideas from
> > > Chris and Dave, but that check would change the desired behavior of the
> > > code, because the object (for example pdp->page_directory[iter]) can be
> > > null during init/alloc, and C would take this as false, breaking the for
> > > loop immediately.
> > > 
> > > This has been already verified with "static analysis tools".
> > > 
> > > [1]http://lists.freedesktop.org/archives/intel-gfx/2015-June/068548.html
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Dave Gordon <david.s.gordon@intel.com>
> > > Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> > 
> > So maybe I'm dense and not seeing what's really going on, but the only
> > thing we seem to be doing is create a pointer to arr[SIZE], i.e. a pointer
> > to the element right after the last valid one. Pointer arithmetic and
> > comparison are explicitly allowed by the C standard on such a pointer. The
> > only thing not allowed is dereference it (which we don't seem to be doing
> > here).
> 
> You're thinking of &(pd)->page_table[iter] (i.e. (pd)->page_table +
> iter). There is an apparent dereference here of (pd)->page_table[ITER_SIZE].

Oh right.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx