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

Submitted by Michel Thierry on Oct. 1, 2015, 3:59 p.m.

Details

Message ID 1443715175-32567-1-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry Oct. 1, 2015, 3:59 p.m.
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>
---
 drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 9fbb07d..94f8344 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -394,7 +394,9 @@  struct i915_hw_ppgtt {
  */
 #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
 	for (iter = gen6_pde_index(start); \
-	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
+	     pt = (length > 0 && iter < I915_PDES) ? \
+			(pd)->page_table[iter] : NULL, \
+	     length > 0 && iter < I915_PDES; \
 	     iter++, \
 	     temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
 	     temp = min_t(unsigned, temp, length), \
@@ -459,7 +461,9 @@  static inline uint32_t gen6_pde_index(uint32_t addr)
  */
 #define gen8_for_each_pde(pt, pd, start, length, temp, iter)		\
 	for (iter = gen8_pde_index(start); \
-	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES;	\
+	     pt = (length > 0 && iter < I915_PDES) ? \
+			(pd)->page_table[iter] : NULL, \
+	     length > 0 && iter < I915_PDES;	\
 	     iter++,				\
 	     temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,	\
 	     temp = min(temp, length),					\
@@ -467,7 +471,8 @@  static inline uint32_t gen6_pde_index(uint32_t addr)
 
 #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)	\
 	for (iter = gen8_pdpe_index(start); \
-	     pd = (pdp)->page_directory[iter], \
+	     pd = (length > 0 && (iter < I915_PDPES_PER_PDP(dev))) ? \
+			(pdp)->page_directory[iter] : NULL, \
 	     length > 0 && (iter < I915_PDPES_PER_PDP(dev)); \
 	     iter++,				\
 	     temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,	\
@@ -476,7 +481,8 @@  static inline uint32_t gen6_pde_index(uint32_t addr)
 
 #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)	\
 	for (iter = gen8_pml4e_index(start);	\
-	     pdp = (pml4)->pdps[iter], \
+	     pdp = (length > 0 && iter < GEN8_PML4ES_PER_PML4) ? \
+			(pml4)->pdps[iter] : NULL, \
 	     length > 0 && iter < GEN8_PML4ES_PER_PML4; \
 	     iter++,				\
 	     temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,	\

Comments

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>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d..94f8344 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>   */
>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>  	for (iter = gen6_pde_index(start); \
> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> +	     pt = (length > 0 && iter < I915_PDES) ? \
> +			(pd)->page_table[iter] : NULL, \
> +	     length > 0 && iter < I915_PDES; \

length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,

as the compiler wouldn't be able to CSE it otherwise (I think).
-Chris
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).

The reason for this is exactly this case here, not allowing this means
checking whether you've run off the end of an array often becomes very
unnatural.

So if there's nothing else going on I think the right choice is to mark
them all up as false positives - the tool is wrong.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 9fbb07d..94f8344 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>   */
>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>  	for (iter = gen6_pde_index(start); \
> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> +	     pt = (length > 0 && iter < I915_PDES) ? \
> +			(pd)->page_table[iter] : NULL, \
> +	     length > 0 && iter < I915_PDES; \
>  	     iter++, \
>  	     temp = ALIGN(start+1, 1 << GEN6_PDE_SHIFT) - start, \
>  	     temp = min_t(unsigned, temp, length), \
> @@ -459,7 +461,9 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>   */
>  #define gen8_for_each_pde(pt, pd, start, length, temp, iter)		\
>  	for (iter = gen8_pde_index(start); \
> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES;	\
> +	     pt = (length > 0 && iter < I915_PDES) ? \
> +			(pd)->page_table[iter] : NULL, \
> +	     length > 0 && iter < I915_PDES;	\
>  	     iter++,				\
>  	     temp = ALIGN(start+1, 1 << GEN8_PDE_SHIFT) - start,	\
>  	     temp = min(temp, length),					\
> @@ -467,7 +471,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>  
>  #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)	\
>  	for (iter = gen8_pdpe_index(start); \
> -	     pd = (pdp)->page_directory[iter], \
> +	     pd = (length > 0 && (iter < I915_PDPES_PER_PDP(dev))) ? \
> +			(pdp)->page_directory[iter] : NULL, \
>  	     length > 0 && (iter < I915_PDPES_PER_PDP(dev)); \
>  	     iter++,				\
>  	     temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,	\
> @@ -476,7 +481,8 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>  
>  #define gen8_for_each_pml4e(pdp, pml4, start, length, temp, iter)	\
>  	for (iter = gen8_pml4e_index(start);	\
> -	     pdp = (pml4)->pdps[iter], \
> +	     pdp = (length > 0 && iter < GEN8_PML4ES_PER_PML4) ? \
> +			(pml4)->pdps[iter] : NULL, \
>  	     length > 0 && iter < GEN8_PML4ES_PER_PML4; \
>  	     iter++,				\
>  	     temp = ALIGN(start+1, 1ULL << GEN8_PML4E_SHIFT) - start,	\
> -- 
> 2.6.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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].
-Chris
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
On 10/1/2015 5:09 PM, Chris Wilson wrote:
> On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index 9fbb07d..94f8344 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>>    */
>>   #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>>   	for (iter = gen6_pde_index(start); \
>> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
>> +	     pt = (length > 0 && iter < I915_PDES) ? \
>> +			(pd)->page_table[iter] : NULL, \
>> +	     length > 0 && iter < I915_PDES; \
>
> length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,
>
> as the compiler wouldn't be able to CSE it otherwise (I think).

Even after that change, the compiler keeps doing an optimization when 
page_table[iter] is null (takes the null assignment as the break condition).

I've been playing with these examples
http://paste.ubuntu.com/12638106/

Only the 1st example (a) iterates over all elements, b & c stop after 
the 1st run.

Thanks,

-Michel
On Fri, Oct 02, 2015 at 01:47:03PM +0100, Michel Thierry wrote:
> On 10/1/2015 5:09 PM, Chris Wilson wrote:
> >On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
> >>---
> >>  drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>index 9fbb07d..94f8344 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >>@@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
> >>   */
> >>  #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
> >>  	for (iter = gen6_pde_index(start); \
> >>-	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
> >>+	     pt = (length > 0 && iter < I915_PDES) ? \
> >>+			(pd)->page_table[iter] : NULL, \
> >>+	     length > 0 && iter < I915_PDES; \
> >
> >length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,
> >
> >as the compiler wouldn't be able to CSE it otherwise (I think).
> 
> Even after that change, the compiler keeps doing an optimization
> when page_table[iter] is null (takes the null assignment as the
> break condition).
> 
> I've been playing with these examples
> http://paste.ubuntu.com/12638106/
> 
> Only the 1st example (a) iterates over all elements, b & c stop
> after the 1st run.

Forgot that was the condition you wanted to change.

length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]), 1 : 0,

Would be nice to prove that length > 0 implies iter < I915_PDES. If only
we had smart tools :)
-Chris
On 10/2/2015 1:58 PM, Chris Wilson wrote:
> On Fri, Oct 02, 2015 at 01:47:03PM +0100, Michel Thierry wrote:
>> On 10/1/2015 5:09 PM, Chris Wilson wrote:
>>> On Thu, Oct 01, 2015 at 04:59:35PM +0100, Michel Thierry wrote:
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_gem_gtt.h | 14 ++++++++++----
>>>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>> index 9fbb07d..94f8344 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>>>> @@ -394,7 +394,9 @@ struct i915_hw_ppgtt {
>>>>    */
>>>>   #define gen6_for_each_pde(pt, pd, start, length, temp, iter) \
>>>>   	for (iter = gen6_pde_index(start); \
>>>> -	     pt = (pd)->page_table[iter], length > 0 && iter < I915_PDES; \
>>>> +	     pt = (length > 0 && iter < I915_PDES) ? \
>>>> +			(pd)->page_table[iter] : NULL, \
>>>> +	     length > 0 && iter < I915_PDES; \
>>>
>>> length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]) : 0,
>>>
>>> as the compiler wouldn't be able to CSE it otherwise (I think).
>>
>> Even after that change, the compiler keeps doing an optimization
>> when page_table[iter] is null (takes the null assignment as the
>> break condition).
>>
>> I've been playing with these examples
>> http://paste.ubuntu.com/12638106/
>>
>> Only the 1st example (a) iterates over all elements, b & c stop
>> after the 1st run.
>
> Forgot that was the condition you wanted to change.
>
> length > 0 && iter < I915_PDES ? (pt = (pd)->page_table[iter]), 1 : 0,
>
> Would be nice to prove that length > 0 implies iter < I915_PDES. If only
> we had smart tools :)

Thanks, that made it.