linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
@ 2015-05-25 10:29 Andrey Ryabinin
  2015-05-25 13:12 ` Jani Nikula
  2015-05-26 15:32 ` Daniel Vetter
  0 siblings, 2 replies; 8+ messages in thread
From: Andrey Ryabinin @ 2015-05-25 10:29 UTC (permalink / raw)
  To: David Airlie
  Cc: Ander Conselvan de Oliveira, Andrey Ryabinin, dri-devel, linux-kernel

for_each_*_in_state validate array index after
access to array elements, thus perform out of bounds read.

Fix this by validating index in the first place and read
array element iff validation was successful.

Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
---
 include/drm/drm_atomic.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index c1571034..3f13b91 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
 
 #define for_each_connector_in_state(state, connector, connector_state, __i) \
 	for ((__i) = 0;							\
-	     (connector) = (state)->connectors[__i],			\
-	     (connector_state) = (state)->connector_states[__i],	\
-	     (__i) < (state)->num_connector;				\
+	     (__i) < (state)->num_connector &&				\
+	     ((connector) = (state)->connectors[__i],			\
+	     (connector_state) = (state)->connector_states[__i], 1); 	\
 	     (__i)++)							\
 		if (connector)
 
 #define for_each_crtc_in_state(state, crtc, crtc_state, __i)	\
 	for ((__i) = 0;						\
-	     (crtc) = (state)->crtcs[__i],			\
-	     (crtc_state) = (state)->crtc_states[__i],		\
-	     (__i) < (state)->dev->mode_config.num_crtc;	\
+	     (__i) < (state)->dev->mode_config.num_crtc &&	\
+	     ((crtc) = (state)->crtcs[__i],			\
+	     (crtc_state) = (state)->crtc_states[__i], 1);	\
 	     (__i)++)						\
 		if (crtc_state)
 
-#define for_each_plane_in_state(state, plane, plane_state, __i)	\
-	for ((__i) = 0;						\
-	     (plane) = (state)->planes[__i],			\
-	     (plane_state) = (state)->plane_states[__i],	\
-	     (__i) < (state)->dev->mode_config.num_total_plane;	\
-	     (__i)++)						\
+#define for_each_plane_in_state(state, plane, plane_state, __i)		\
+	for ((__i) = 0;							\
+	     (__i) < (state)->dev->mode_config.num_total_plane &&	\
+	     ((plane) = (state)->planes[__i],				\
+	     (plane_state) = (state)->plane_states[__i], 1);		\
+	     (__i)++)							\
 		if (plane_state)
 
 #endif /* DRM_ATOMIC_H_ */
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 10:29 [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers Andrey Ryabinin
@ 2015-05-25 13:12 ` Jani Nikula
  2015-05-25 13:25   ` Andrey Ryabinin
  2015-05-26 15:32 ` Daniel Vetter
  1 sibling, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2015-05-25 13:12 UTC (permalink / raw)
  To: Andrey Ryabinin, David Airlie
  Cc: Andrey Ryabinin, Ander Conselvan de Oliveira, linux-kernel, dri-devel

On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> for_each_*_in_state validate array index after
> access to array elements, thus perform out of bounds read.
>
> Fix this by validating index in the first place and read
> array element iff validation was successful.
>
> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> ---
>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index c1571034..3f13b91 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>  
>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>  	for ((__i) = 0;							\
> -	     (connector) = (state)->connectors[__i],			\
> -	     (connector_state) = (state)->connector_states[__i],	\
> -	     (__i) < (state)->num_connector;				\
> +	     (__i) < (state)->num_connector &&				\
> +	     ((connector) = (state)->connectors[__i],			\
> +	     (connector_state) = (state)->connector_states[__i], 1); 	\

This will stop at the first NULL connector/connector_state. Similarly
for the loops below.

BR,
Jani.

>  	     (__i)++)							\
>  		if (connector)
>  
>  #define for_each_crtc_in_state(state, crtc, crtc_state, __i)	\
>  	for ((__i) = 0;						\
> -	     (crtc) = (state)->crtcs[__i],			\
> -	     (crtc_state) = (state)->crtc_states[__i],		\
> -	     (__i) < (state)->dev->mode_config.num_crtc;	\
> +	     (__i) < (state)->dev->mode_config.num_crtc &&	\
> +	     ((crtc) = (state)->crtcs[__i],			\
> +	     (crtc_state) = (state)->crtc_states[__i], 1);	\
>  	     (__i)++)						\
>  		if (crtc_state)
>  
> -#define for_each_plane_in_state(state, plane, plane_state, __i)	\
> -	for ((__i) = 0;						\
> -	     (plane) = (state)->planes[__i],			\
> -	     (plane_state) = (state)->plane_states[__i],	\
> -	     (__i) < (state)->dev->mode_config.num_total_plane;	\
> -	     (__i)++)						\
> +#define for_each_plane_in_state(state, plane, plane_state, __i)		\
> +	for ((__i) = 0;							\
> +	     (__i) < (state)->dev->mode_config.num_total_plane &&	\
> +	     ((plane) = (state)->planes[__i],				\
> +	     (plane_state) = (state)->plane_states[__i], 1);		\
> +	     (__i)++)							\
>  		if (plane_state)
>  
>  #endif /* DRM_ATOMIC_H_ */
> -- 
> 2.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 13:12 ` Jani Nikula
@ 2015-05-25 13:25   ` Andrey Ryabinin
  2015-05-25 13:50     ` Jani Nikula
  0 siblings, 1 reply; 8+ messages in thread
From: Andrey Ryabinin @ 2015-05-25 13:25 UTC (permalink / raw)
  To: Jani Nikula, David Airlie
  Cc: Ander Conselvan de Oliveira, linux-kernel, dri-devel

On 05/25/2015 04:12 PM, Jani Nikula wrote:
> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>> for_each_*_in_state validate array index after
>> access to array elements, thus perform out of bounds read.
>>
>> Fix this by validating index in the first place and read
>> array element iff validation was successful.
>>
>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>> ---
>>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index c1571034..3f13b91 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>>  
>>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>>  	for ((__i) = 0;							\
>> -	     (connector) = (state)->connectors[__i],			\
>> -	     (connector_state) = (state)->connector_states[__i],	\
>> -	     (__i) < (state)->num_connector;				\
>> +	     (__i) < (state)->num_connector &&				\
>> +	     ((connector) = (state)->connectors[__i],			\
>> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
> 
> This will stop at the first NULL connector/connector_state. Similarly
> for the loops below.
> 

This will stop iff (__i) >= (state)->num_connector, because the result of expression:
	 ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1)
is always 1.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 13:25   ` Andrey Ryabinin
@ 2015-05-25 13:50     ` Jani Nikula
  2015-05-25 13:52       ` Jani Nikula
  0 siblings, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2015-05-25 13:50 UTC (permalink / raw)
  To: Andrey Ryabinin, David Airlie
  Cc: Ander Conselvan de Oliveira, linux-kernel, dri-devel

On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> On 05/25/2015 04:12 PM, Jani Nikula wrote:
>> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>>> for_each_*_in_state validate array index after
>>> access to array elements, thus perform out of bounds read.
>>>
>>> Fix this by validating index in the first place and read
>>> array element iff validation was successful.
>>>
>>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
>>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>>> ---
>>>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>> index c1571034..3f13b91 100644
>>> --- a/include/drm/drm_atomic.h
>>> +++ b/include/drm/drm_atomic.h
>>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>>>  
>>>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>>>  	for ((__i) = 0;							\
>>> -	     (connector) = (state)->connectors[__i],			\
>>> -	     (connector_state) = (state)->connector_states[__i],	\
>>> -	     (__i) < (state)->num_connector;				\
>>> +	     (__i) < (state)->num_connector &&				\
>>> +	     ((connector) = (state)->connectors[__i],			\
>>> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
>> 
>> This will stop at the first NULL connector/connector_state. Similarly
>> for the loops below.
>> 
>
> This will stop iff (__i) >= (state)->num_connector, because the result of expression:
> 	 ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1)
> is always 1.

Why do you think it'll always be 1?

BR,
Jani.


>
>

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 13:50     ` Jani Nikula
@ 2015-05-25 13:52       ` Jani Nikula
  2015-05-25 13:57         ` Ville Syrjälä
  2015-05-25 14:03         ` Jani Nikula
  0 siblings, 2 replies; 8+ messages in thread
From: Jani Nikula @ 2015-05-25 13:52 UTC (permalink / raw)
  To: Andrey Ryabinin, David Airlie
  Cc: Ander Conselvan de Oliveira, linux-kernel, dri-devel

On Mon, 25 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>> On 05/25/2015 04:12 PM, Jani Nikula wrote:
>>> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>>>> for_each_*_in_state validate array index after
>>>> access to array elements, thus perform out of bounds read.
>>>>
>>>> Fix this by validating index in the first place and read
>>>> array element iff validation was successful.
>>>>
>>>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
>>>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>>>> ---
>>>>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>>>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>> index c1571034..3f13b91 100644
>>>> --- a/include/drm/drm_atomic.h
>>>> +++ b/include/drm/drm_atomic.h
>>>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>>>>  
>>>>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>>>>  	for ((__i) = 0;							\
>>>> -	     (connector) = (state)->connectors[__i],			\
>>>> -	     (connector_state) = (state)->connector_states[__i],	\
>>>> -	     (__i) < (state)->num_connector;				\
>>>> +	     (__i) < (state)->num_connector &&				\
>>>> +	     ((connector) = (state)->connectors[__i],			\
>>>> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
>>> 
>>> This will stop at the first NULL connector/connector_state. Similarly
>>> for the loops below.
>>> 
>>
>> This will stop iff (__i) >= (state)->num_connector, because the result of expression:
>> 	 ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1)
>> is always 1.
>
> Why do you think it'll always be 1?

That might be because there's the 1 at the end. *blush*.

I do wonder if this is too subtle in general, or if it's just too subtle
for me.

BR,
Jani.

>
> BR,
> Jani.
>
>
>>
>>
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 13:52       ` Jani Nikula
@ 2015-05-25 13:57         ` Ville Syrjälä
  2015-05-25 14:03         ` Jani Nikula
  1 sibling, 0 replies; 8+ messages in thread
From: Ville Syrjälä @ 2015-05-25 13:57 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Andrey Ryabinin, David Airlie, Ander Conselvan de Oliveira,
	linux-kernel, dri-devel

On Mon, May 25, 2015 at 04:52:31PM +0300, Jani Nikula wrote:
> On Mon, 25 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> >> On 05/25/2015 04:12 PM, Jani Nikula wrote:
> >>> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
> >>>> for_each_*_in_state validate array index after
> >>>> access to array elements, thus perform out of bounds read.
> >>>>
> >>>> Fix this by validating index in the first place and read
> >>>> array element iff validation was successful.
> >>>>
> >>>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
> >>>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> >>>> ---
> >>>>  include/drm/drm_atomic.h | 24 ++++++++++++------------
> >>>>  1 file changed, 12 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >>>> index c1571034..3f13b91 100644
> >>>> --- a/include/drm/drm_atomic.h
> >>>> +++ b/include/drm/drm_atomic.h
> >>>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
> >>>>  
> >>>>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
> >>>>  	for ((__i) = 0;							\
> >>>> -	     (connector) = (state)->connectors[__i],			\
> >>>> -	     (connector_state) = (state)->connector_states[__i],	\
> >>>> -	     (__i) < (state)->num_connector;				\
> >>>> +	     (__i) < (state)->num_connector &&				\
> >>>> +	     ((connector) = (state)->connectors[__i],			\
> >>>> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
> >>> 
> >>> This will stop at the first NULL connector/connector_state. Similarly
> >>> for the loops below.
> >>> 
> >>
> >> This will stop iff (__i) >= (state)->num_connector, because the result of expression:
> >> 	 ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1)
> >> is always 1.
> >
> > Why do you think it'll always be 1?
> 
> That might be because there's the 1 at the end. *blush*.
> 
> I do wonder if this is too subtle in general, or if it's just too subtle
> for me.

s/1/true/ might make it a bit less subtle, but not by much.

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 13:52       ` Jani Nikula
  2015-05-25 13:57         ` Ville Syrjälä
@ 2015-05-25 14:03         ` Jani Nikula
  1 sibling, 0 replies; 8+ messages in thread
From: Jani Nikula @ 2015-05-25 14:03 UTC (permalink / raw)
  To: Andrey Ryabinin, David Airlie
  Cc: Ander Conselvan de Oliveira, linux-kernel, dri-devel

On Mon, 25 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 25 May 2015, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>>> On 05/25/2015 04:12 PM, Jani Nikula wrote:
>>>> On Mon, 25 May 2015, Andrey Ryabinin <a.ryabinin@samsung.com> wrote:
>>>>> for_each_*_in_state validate array index after
>>>>> access to array elements, thus perform out of bounds read.
>>>>>
>>>>> Fix this by validating index in the first place and read
>>>>> array element iff validation was successful.
>>>>>
>>>>> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
>>>>> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
>>>>> ---
>>>>>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>>>>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>>> index c1571034..3f13b91 100644
>>>>> --- a/include/drm/drm_atomic.h
>>>>> +++ b/include/drm/drm_atomic.h
>>>>> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>>>>>  
>>>>>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>>>>>  	for ((__i) = 0;							\
>>>>> -	     (connector) = (state)->connectors[__i],			\
>>>>> -	     (connector_state) = (state)->connector_states[__i],	\
>>>>> -	     (__i) < (state)->num_connector;				\
>>>>> +	     (__i) < (state)->num_connector &&				\
>>>>> +	     ((connector) = (state)->connectors[__i],			\
>>>>> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
>>>> 
>>>> This will stop at the first NULL connector/connector_state. Similarly
>>>> for the loops below.
>>>> 
>>>
>>> This will stop iff (__i) >= (state)->num_connector, because the result of expression:
>>> 	 ((connector) = (state)->connectors[__i], (connector_state) = (state)->connector_states[__i], 1)
>>> is always 1.
>>
>> Why do you think it'll always be 1?
>
> That might be because there's the 1 at the end. *blush*.
>
> I do wonder if this is too subtle in general, or if it's just too subtle
> for me.


So in the mean time, I was looking at doing the below, not because of
this patch or the bug it fixes, but because I think the construct

	for (...) \
        	if (...)

in a for_something() style macro is a dangling else disaster waiting to
happen.

It's a bit tedious, I admit, and apparently makes some gcc versions
whine about using uninitialized variables, because they're not smart
enough to realize the pointers are initialized when the loop condition
is met.

BR,
Jani.


diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index e89db0c377ba..eb81f5930a8c 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -134,28 +134,70 @@ int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
 int __must_check drm_atomic_commit(struct drm_atomic_state *state);
 int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
 
+static inline int next_connector(struct drm_connector **pconnector,
+				 struct drm_connector **connectors,
+				 struct drm_connector_state **pstate,
+				 struct drm_connector_state **states,
+				 int index, int max)
+{
+	while (index < max && (connectors[index] == NULL || states[index] == NULL))
+		index++;
+
+	if (index < max) {
+		*pconnector = connectors[index];
+		*pstate = states[index];
+	}
+
+	return index;
+}
+
+static inline int next_crtc(struct drm_crtc **pcrtc,
+			    struct drm_crtc **crtcs,
+			    struct drm_crtc_state **pstate,
+			    struct drm_crtc_state **states,
+			    int index, int max)
+{
+	while (index < max && (crtcs[index] == NULL || states[index] == NULL))
+		index++;
+
+	if (index < max) {
+		*pcrtc = crtcs[index];
+		*pstate = states[index];
+	}
+
+	return index;
+}
+
+static inline int next_plane(struct drm_plane **pplane,
+			    struct drm_plane **planes,
+			    struct drm_plane_state **pstate,
+			    struct drm_plane_state **states,
+			    int index, int max)
+{
+	while (index < max && (planes[index] == NULL || states[index] == NULL))
+		index++;
+
+	if (index < max) {
+		*pplane = planes[index];
+		*pstate = states[index];
+	}
+
+	return index;
+}
+
 #define for_each_connector_in_state(state, connector, connector_state, __i) \
-	for ((__i) = 0;							\
-	     (connector) = (state)->connectors[__i],			\
-	     (connector_state) = (state)->connector_states[__i],	\
+	for ((__i) = next_connector(&(connector), (state)->connectors, &(connector_state), (state)->connector_states, 0, (state)->num_connector); \
 	     (__i) < (state)->num_connector;				\
-	     (__i)++)							\
-		if (connector)
-
-#define for_each_crtc_in_state(state, crtc, crtc_state, __i)	\
-	for ((__i) = 0;						\
-	     (crtc) = (state)->crtcs[__i],			\
-	     (crtc_state) = (state)->crtc_states[__i],		\
-	     (__i) < (state)->dev->mode_config.num_crtc;	\
-	     (__i)++)						\
-		if (crtc_state)
-
-#define for_each_plane_in_state(state, plane, plane_state, __i)	\
-	for ((__i) = 0;						\
-	     (plane) = (state)->planes[__i],			\
-	     (plane_state) = (state)->plane_states[__i],	\
-	     (__i) < (state)->dev->mode_config.num_total_plane;	\
-	     (__i)++)						\
-		if (plane_state)
+	     (__i) = next_connector(&(connector), (state)->connectors, &(connector_state), (state)->connector_states, (__i), (state)->num_connector))
+
+#define for_each_crtc_in_state(state, crtc, crtc_state, __i)		\
+	for ((__i) = next_crtc(&(crtc), (state)->crtcs, &(crtc_state), (state)->crtc_states, 0, (state)->dev->mode_config.num_crtc); \
+	     (__i) < (state)->dev->mode_config.num_crtc;		\
+	     (__i) = next_crtc(&crtc, (state)->crtcs, &(crtc_state), (state)->crtc_states, (__i), (state)->dev->mode_config.num_crtc))
+
+#define for_each_plane_in_state(state, plane, plane_state, __i)		\
+	for ((__i) = next_plane(&(plane), (state)->planes, &(plane_state), (state)->plane_states, 0, (state)->dev->mode_config.num_total_plane); \
+	     (__i) < (state)->dev->mode_config.num_total_plane;		\
+	     (__i) = next_plane(&(plane), (state)->planes, &(plane_state), (state)->plane_states, (__i), (state)->dev->mode_config.num_total_plane))
 
 #endif /* DRM_ATOMIC_H_ */




-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers
  2015-05-25 10:29 [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers Andrey Ryabinin
  2015-05-25 13:12 ` Jani Nikula
@ 2015-05-26 15:32 ` Daniel Vetter
  1 sibling, 0 replies; 8+ messages in thread
From: Daniel Vetter @ 2015-05-26 15:32 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: David Airlie, Ander Conselvan de Oliveira, linux-kernel, dri-devel

On Mon, May 25, 2015 at 01:29:44PM +0300, Andrey Ryabinin wrote:
> for_each_*_in_state validate array index after
> access to array elements, thus perform out of bounds read.
> 
> Fix this by validating index in the first place and read
> array element iff validation was successful.
> 
> Fixes: df63b9994eaf ("drm/atomic: Add for_each_{connector,crtc,plane}_in_state helper macros")
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>

Merged this for now to handle the regression, but yeah some polish might
be good.
-Daniel

> ---
>  include/drm/drm_atomic.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index c1571034..3f13b91 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -77,26 +77,26 @@ int __must_check drm_atomic_async_commit(struct drm_atomic_state *state);
>  
>  #define for_each_connector_in_state(state, connector, connector_state, __i) \
>  	for ((__i) = 0;							\
> -	     (connector) = (state)->connectors[__i],			\
> -	     (connector_state) = (state)->connector_states[__i],	\
> -	     (__i) < (state)->num_connector;				\
> +	     (__i) < (state)->num_connector &&				\
> +	     ((connector) = (state)->connectors[__i],			\
> +	     (connector_state) = (state)->connector_states[__i], 1); 	\
>  	     (__i)++)							\
>  		if (connector)
>  
>  #define for_each_crtc_in_state(state, crtc, crtc_state, __i)	\
>  	for ((__i) = 0;						\
> -	     (crtc) = (state)->crtcs[__i],			\
> -	     (crtc_state) = (state)->crtc_states[__i],		\
> -	     (__i) < (state)->dev->mode_config.num_crtc;	\
> +	     (__i) < (state)->dev->mode_config.num_crtc &&	\
> +	     ((crtc) = (state)->crtcs[__i],			\
> +	     (crtc_state) = (state)->crtc_states[__i], 1);	\
>  	     (__i)++)						\
>  		if (crtc_state)
>  
> -#define for_each_plane_in_state(state, plane, plane_state, __i)	\
> -	for ((__i) = 0;						\
> -	     (plane) = (state)->planes[__i],			\
> -	     (plane_state) = (state)->plane_states[__i],	\
> -	     (__i) < (state)->dev->mode_config.num_total_plane;	\
> -	     (__i)++)						\
> +#define for_each_plane_in_state(state, plane, plane_state, __i)		\
> +	for ((__i) = 0;							\
> +	     (__i) < (state)->dev->mode_config.num_total_plane &&	\
> +	     ((plane) = (state)->planes[__i],				\
> +	     (plane_state) = (state)->plane_states[__i], 1);		\
> +	     (__i)++)							\
>  		if (plane_state)
>  
>  #endif /* DRM_ATOMIC_H_ */
> -- 
> 2.4.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-05-26 15:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-25 10:29 [PATCH] drm/atomic: fix out of bounds read in for_each_*_in_state helpers Andrey Ryabinin
2015-05-25 13:12 ` Jani Nikula
2015-05-25 13:25   ` Andrey Ryabinin
2015-05-25 13:50     ` Jani Nikula
2015-05-25 13:52       ` Jani Nikula
2015-05-25 13:57         ` Ville Syrjälä
2015-05-25 14:03         ` Jani Nikula
2015-05-26 15:32 ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).