[PATCH] drm/komeda: Fix bit check to import to value of proper type

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

Another issue found by KASAN. The bit finding is bueried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_component_state *c_st;
 	struct komeda_component *c;
-	u32 disabling_comps, id;
+	u32 id;
+	unsigned long disabling_comps;
 
 	WARN_ON(!old);
 
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe,
 		st = komeda_pipeline_get_new_state(pipe, drm_st);
 	else
 		st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
-
 	if (WARN_ON(IS_ERR_OR_NULL(st)))
 		return -EINVAL;
 
@@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
 	struct komeda_component_state *c_st;
-	u32 id, disabling_comps = 0;
+	u32 id;
+	unsigned long disabling_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
@@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 		disabling_comps = old->active_comps &
 				  pipe->standalone_disabled_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
 			 pipe->id, old->active_comps, disabling_comps);
 
 	dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
-	u32 id, changed_comps = 0;
+	u32 id;
+	unsigned long changed_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
 	changed_comps = new->active_comps | old->active_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
 			 pipe->id, new->active_comps, changed_comps);
 
 	dp_for_each_set_bit(id, changed_comps) {
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Steven Price]

On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Another issue found by KASAN. The bit finding is bueried inside the

NIT: s/bueried/buried/

> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
> calls the bit stuff. These bit functions want an unsigned long pointer
> as input and just dumbly casting leads to out-of-bounds accesses.
> This fixes that.

This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:

   #define dp_for_each_set_bit(bit, mask) \
   	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)

i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.

So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.

Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.

The other option is to fix dp_for_each_set_bit() itself using a little hack:

-       for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
+       for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) * 8)

With that I don't think you need any other change as the mask is actually
in a new unsigned long on the stack.

Steve

> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index e8b1e15312d8..f7dddb9f015d 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_component_state *c_st;
>   	struct komeda_component *c;
> -	u32 disabling_comps, id;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	WARN_ON(!old);
>   
> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe,
>   		st = komeda_pipeline_get_new_state(pipe, drm_st);
>   	else
>   		st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
> -

NIT: Random white space change

>   	if (WARN_ON(IS_ERR_OR_NULL(st)))
>   		return -EINVAL;
>   
> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
>   	struct komeda_component_state *c_st;
> -	u32 id, disabling_comps = 0;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   		disabling_comps = old->active_comps &
>   				  pipe->standalone_disabled_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
>   			 pipe->id, old->active_comps, disabling_comps);
>   
>   	dp_for_each_set_bit(id, disabling_comps) {
> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
> -	u32 id, changed_comps = 0;
> +	u32 id;
> +	unsigned long changed_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
>   	changed_comps = new->active_comps | old->active_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>   			 pipe->id, new->active_comps, changed_comps);
>   
>   	dp_for_each_set_bit(id, changed_comps) {
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/20/21 3:44 PM, Steven Price wrote:
> On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>
>> Another issue found by KASAN. The bit finding is bueried inside the
> 
> NIT: s/bueried/buried/

Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
whitespace change along the way so can fix both in a new patch.

>> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
>> calls the bit stuff. These bit functions want an unsigned long pointer
>> as input and just dumbly casting leads to out-of-bounds accesses.
>> This fixes that.
> 
> This seems like an underlying bug/lack of clear documentation for the
> underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
> right thing:

Correct. This was a general problem I spotted - the bit funcs were 
written to want a unsigned long but were being used on u32's by just 
casting the ptr to the type and this did indeed have technical issues.

>    #define dp_for_each_set_bit(bit, mask) \
>        for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
> * 8)
> 
> i.e. passing the actual size of type. However because of the case to
> unsigned long (and subsequent accesses using that type) the compiler is
> free to make accesses beyond the size of the variable (and indeed this
> is completely broken on big-endian). The implementation actually
> requires that the bitmap is really an array of unsigned long - no other
> type will work.

Precisely. So a bit loose. The bit funcs are used widely enough, so just 
fixing this code here to pass in the expected type is probably the least 
disruptive fix.

> So I think the fix should also include fixing the dp_for_each_set_bit()
> macro - the cast is bogus as it stands.

Yup. Removing the cast does indeed find more badness that needs fixing. 
I'll do an updated patch with this.

> Doing that I also get warnings on komeda_pipeline::avail_comps and
> komeda_pipeline::supported_inputs - although I note there are other
> bitmasks mentioned.
> 
> The other option is to fix dp_for_each_set_bit() itself using a little 
> hack:
> 
> -       for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
> * 8)
> +       for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) 
> * 8)
> 
> With that I don't think you need any other change as the mask is actually
> in a new unsigned long on the stack.

That would be wonderful if it worked :). Unfortunately your above 
proposal leads to:

./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
error: lvalue required as unary ‘&’ operand
    17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)
       |                           ^
./include/linux/bitops.h:34:30: note: in definition of macro 
‘for_each_set_bit’
    34 |       (bit) = find_next_bit((addr), (size), (bit) + 1))
       |                              ^~~~
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: 
in expansion of macro ‘dp_for_each_set_bit’
  1243 |  dp_for_each_set_bit(id, disabling_comps) {
       |  ^~~~~~~~~~~~~~~~~~~

Basically can't take address of an "unnamed local var". :| That is with:

#define dp_for_each_set_bit(bit, mask) \
         for_each_set_bit((bit), (&((unsigned long)(mask))), 
sizeof(mask) * 8)

I could try have the dp_for_each macro create new local vars on its own 
a bit like:

#define dp_for_each_set_bit(bit, mask) \
         unsigned long __local_mask = mask; \
         for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)

But we all know where this leads... (multiple bad places with adding 
warnings and potential and pitfalls that then lead with ever more 
invasive changes to address like if code in future might do if (x) 
dp_for_each...). I'd prefer to be able to write code more loosely (pass 
in any time and it just does the right thing), but trying to balance 
this with least disruption and ugliness.

> Steve
> 
>>
>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>> ---
>>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git 
>> a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
>> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> index e8b1e15312d8..f7dddb9f015d 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *old = 
>> priv_to_pipe_st(pipe->obj.state);
>>       struct komeda_component_state *c_st;
>>       struct komeda_component *c;
>> -    u32 disabling_comps, id;
>> +    u32 id;
>> +    unsigned long disabling_comps;
>>       WARN_ON(!old);
>> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
>> komeda_pipeline *pipe,
>>           st = komeda_pipeline_get_new_state(pipe, drm_st);
>>       else
>>           st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
>> NULL);
>> -
> 
> NIT: Random white space change
> 
>>       if (WARN_ON(IS_ERR_OR_NULL(st)))
>>           return -EINVAL;
>> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *old;
>>       struct komeda_component *c;
>>       struct komeda_component_state *c_st;
>> -    u32 id, disabling_comps = 0;
>> +    u32 id;
>> +    unsigned long disabling_comps;
>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
>> komeda_pipeline *pipe,
>>           disabling_comps = old->active_comps &
>>                     pipe->standalone_disabled_comps;
>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>> 0x%x.\n",
>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>> 0x%lx.\n",
>>                pipe->id, old->active_comps, disabling_comps);
>>       dp_for_each_set_bit(id, disabling_comps) {
>> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *new = 
>> priv_to_pipe_st(pipe->obj.state);
>>       struct komeda_pipeline_state *old;
>>       struct komeda_component *c;
>> -    u32 id, changed_comps = 0;
>> +    u32 id;
>> +    unsigned long changed_comps;
>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>       changed_comps = new->active_comps | old->active_comps;
>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>>                pipe->id, new->active_comps, changed_comps);
>>       dp_for_each_set_bit(id, changed_comps) {
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Steven Price]

On 21/01/2021 12:22, Carsten Haitzler wrote:
> On 1/20/21 3:44 PM, Steven Price wrote:
>> On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
>>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>>
>>> Another issue found by KASAN. The bit finding is bueried inside the
>>
>> NIT: s/bueried/buried/
> 
> Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
> whitespace change along the way so can fix both in a new patch.
> 
>>> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
>>> calls the bit stuff. These bit functions want an unsigned long pointer
>>> as input and just dumbly casting leads to out-of-bounds accesses.
>>> This fixes that.
>>
>> This seems like an underlying bug/lack of clear documentation for the
>> underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
>> right thing:
> 
> Correct. This was a general problem I spotted - the bit funcs were 
> written to want a unsigned long but were being used on u32's by just 
> casting the ptr to the type and this did indeed have technical issues.
> 
>>    #define dp_for_each_set_bit(bit, mask) \
>>        for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>> sizeof(mask) * 8)
>>
>> i.e. passing the actual size of type. However because of the case to
>> unsigned long (and subsequent accesses using that type) the compiler is
>> free to make accesses beyond the size of the variable (and indeed this
>> is completely broken on big-endian). The implementation actually
>> requires that the bitmap is really an array of unsigned long - no other
>> type will work.
> 
> Precisely. So a bit loose. The bit funcs are used widely enough, so just 
> fixing this code here to pass in the expected type is probably the least 
> disruptive fix.
> 
>> So I think the fix should also include fixing the dp_for_each_set_bit()
>> macro - the cast is bogus as it stands.
> 
> Yup. Removing the cast does indeed find more badness that needs fixing. 
> I'll do an updated patch with this.
> 
>> Doing that I also get warnings on komeda_pipeline::avail_comps and
>> komeda_pipeline::supported_inputs - although I note there are other
>> bitmasks mentioned.
>>
>> The other option is to fix dp_for_each_set_bit() itself using a little 
>> hack:
>>
>> -       for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>> sizeof(mask) * 8)
>> +       for_each_set_bit((bit), (&((unsigned long){mask})), 
>> sizeof(mask) * 8)
>>
>> With that I don't think you need any other change as the mask is actually
>> in a new unsigned long on the stack.
> 
> That would be wonderful if it worked :). Unfortunately your above 
> proposal leads to:
> 
> ./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
> error: lvalue required as unary ‘&’ operand
>     17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
> sizeof(mask) * 8)
>        |                           ^

Looks like you didn't notice the subtle change above. My change uses 
braces ('{}') around 'mask' - I believe it's a GCC extension ("compound 
literals") and it creates an lvalue so you can then take the address of 
it...

I'm not sure if it's a good approach to the problem or not. The 
alternative is to fix up various places to use unsigned longs so you can 
use the unwrapped for_each_set_bit().

Steve

> ./include/linux/bitops.h:34:30: note: in definition of macro 
> ‘for_each_set_bit’
>     34 |       (bit) = find_next_bit((addr), (size), (bit) + 1))
>        |                              ^~~~
> drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: 
> in expansion of macro ‘dp_for_each_set_bit’
>   1243 |  dp_for_each_set_bit(id, disabling_comps) {
>        |  ^~~~~~~~~~~~~~~~~~~
> 
> Basically can't take address of an "unnamed local var". :| That is with:
> 
> #define dp_for_each_set_bit(bit, mask) \
>          for_each_set_bit((bit), (&((unsigned long)(mask))), 
> sizeof(mask) * 8)
> 
> I could try have the dp_for_each macro create new local vars on its own 
> a bit like:
> 
> #define dp_for_each_set_bit(bit, mask) \
>          unsigned long __local_mask = mask; \
>          for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
> 
> But we all know where this leads... (multiple bad places with adding 
> warnings and potential and pitfalls that then lead with ever more 
> invasive changes to address like if code in future might do if (x) 
> dp_for_each...). I'd prefer to be able to write code more loosely (pass 
> in any time and it just does the right thing), but trying to balance 
> this with least disruption and ugliness.
> 
>> Steve
>>
>>>
>>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>>> ---
>>>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git 
>>> a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
>>> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> index e8b1e15312d8..f7dddb9f015d 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *old = 
>>> priv_to_pipe_st(pipe->obj.state);
>>>       struct komeda_component_state *c_st;
>>>       struct komeda_component *c;
>>> -    u32 disabling_comps, id;
>>> +    u32 id;
>>> +    unsigned long disabling_comps;
>>>       WARN_ON(!old);
>>> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
>>> komeda_pipeline *pipe,
>>>           st = komeda_pipeline_get_new_state(pipe, drm_st);
>>>       else
>>>           st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
>>> NULL);
>>> -
>>
>> NIT: Random white space change
>>
>>>       if (WARN_ON(IS_ERR_OR_NULL(st)))
>>>           return -EINVAL;
>>> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *old;
>>>       struct komeda_component *c;
>>>       struct komeda_component_state *c_st;
>>> -    u32 id, disabling_comps = 0;
>>> +    u32 id;
>>> +    unsigned long disabling_comps;
>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
>>> komeda_pipeline *pipe,
>>>           disabling_comps = old->active_comps &
>>>                     pipe->standalone_disabled_comps;
>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>> 0x%x.\n",
>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>> 0x%lx.\n",
>>>                pipe->id, old->active_comps, disabling_comps);
>>>       dp_for_each_set_bit(id, disabling_comps) {
>>> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
>>> komeda_pipeline *pipe,
>>>       struct komeda_pipeline_state *new = 
>>> priv_to_pipe_st(pipe->obj.state);
>>>       struct komeda_pipeline_state *old;
>>>       struct komeda_component *c;
>>> -    u32 id, changed_comps = 0;
>>> +    u32 id;
>>> +    unsigned long changed_comps;
>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>>       changed_comps = new->active_comps | old->active_comps;
>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>>>                pipe->id, new->active_comps, changed_comps);
>>>       dp_for_each_set_bit(id, changed_comps) {
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/21/21 4:40 PM, Steven Price wrote:
> On 21/01/2021 12:22, Carsten Haitzler wrote:
>> On 1/20/21 3:44 PM, Steven Price wrote:
>>> On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
>>>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>>>
>>>> Another issue found by KASAN. The bit finding is bueried inside the
>>>
>>> NIT: s/bueried/buried/
>>
>> Yup. typo not spotted by me. Thanks. Also - i spotted an accidental 
>> whitespace change along the way so can fix both in a new patch.
>>
>>>> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
>>>> calls the bit stuff. These bit functions want an unsigned long pointer
>>>> as input and just dumbly casting leads to out-of-bounds accesses.
>>>> This fixes that.
>>>
>>> This seems like an underlying bug/lack of clear documentation for the
>>> underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
>>> right thing:
>>
>> Correct. This was a general problem I spotted - the bit funcs were 
>> written to want a unsigned long but were being used on u32's by just 
>> casting the ptr to the type and this did indeed have technical issues.
>>
>>>    #define dp_for_each_set_bit(bit, mask) \
>>>        for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>>> sizeof(mask) * 8)
>>>
>>> i.e. passing the actual size of type. However because of the case to
>>> unsigned long (and subsequent accesses using that type) the compiler is
>>> free to make accesses beyond the size of the variable (and indeed this
>>> is completely broken on big-endian). The implementation actually
>>> requires that the bitmap is really an array of unsigned long - no other
>>> type will work.
>>
>> Precisely. So a bit loose. The bit funcs are used widely enough, so 
>> just fixing this code here to pass in the expected type is probably 
>> the least disruptive fix.
>>
>>> So I think the fix should also include fixing the dp_for_each_set_bit()
>>> macro - the cast is bogus as it stands.
>>
>> Yup. Removing the cast does indeed find more badness that needs 
>> fixing. I'll do an updated patch with this.
>>
>>> Doing that I also get warnings on komeda_pipeline::avail_comps and
>>> komeda_pipeline::supported_inputs - although I note there are other
>>> bitmasks mentioned.
>>>
>>> The other option is to fix dp_for_each_set_bit() itself using a 
>>> little hack:
>>>
>>> -       for_each_set_bit((bit), ((unsigned long *)&(mask)), 
>>> sizeof(mask) * 8)
>>> +       for_each_set_bit((bit), (&((unsigned long){mask})), 
>>> sizeof(mask) * 8)
>>>
>>> With that I don't think you need any other change as the mask is 
>>> actually
>>> in a new unsigned long on the stack.
>>
>> That would be wonderful if it worked :). Unfortunately your above 
>> proposal leads to:
>>
>> ./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: 
>> error: lvalue required as unary ‘&’ operand
>>     17 |  for_each_set_bit((bit), (&((unsigned long)(mask))), 
>> sizeof(mask) * 8)
>>        |                           ^
> 
> Looks like you didn't notice the subtle change above. My change uses 
> braces ('{}') around 'mask' - I believe it's a GCC extension ("compound 
> literals") and it creates an lvalue so you can then take the address of 
> it...

Oh... ewww. I did indeed skip the {}'s and just looked at them as ()'s 
:) I'm not so hot on using such extensions if it can be avoided. :) I 
just removed the cast and fixed up all the usages. Patch will come with 
this.

> I'm not sure if it's a good approach to the problem or not. The 
> alternative is to fix up various places to use unsigned longs so you can 
> use the unwrapped for_each_set_bit().
> 
> Steve
> 
>> ./include/linux/bitops.h:34:30: note: in definition of macro 
>> ‘for_each_set_bit’
>>     34 |       (bit) = find_next_bit((addr), (size), (bit) + 1))
>>        |                              ^~~~
>> drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: 
>> note: in expansion of macro ‘dp_for_each_set_bit’
>>   1243 |  dp_for_each_set_bit(id, disabling_comps) {
>>        |  ^~~~~~~~~~~~~~~~~~~
>>
>> Basically can't take address of an "unnamed local var". :| That is with:
>>
>> #define dp_for_each_set_bit(bit, mask) \
>>          for_each_set_bit((bit), (&((unsigned long)(mask))), 
>> sizeof(mask) * 8)
>>
>> I could try have the dp_for_each macro create new local vars on its 
>> own a bit like:
>>
>> #define dp_for_each_set_bit(bit, mask) \
>>          unsigned long __local_mask = mask; \
>>          for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
>>
>> But we all know where this leads... (multiple bad places with adding 
>> warnings and potential and pitfalls that then lead with ever more 
>> invasive changes to address like if code in future might do if (x) 
>> dp_for_each...). I'd prefer to be able to write code more loosely 
>> (pass in any time and it just does the right thing), but trying to 
>> balance this with least disruption and ugliness.
>>
>>> Steve
>>>
>>>>
>>>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>>>> ---
>>>>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 
>>>> ++++++++------
>>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git 
>>>> a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
>>>> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>>> index e8b1e15312d8..f7dddb9f015d 100644
>>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>>>> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
>>>> komeda_pipeline *pipe,
>>>>       struct komeda_pipeline_state *old = 
>>>> priv_to_pipe_st(pipe->obj.state);
>>>>       struct komeda_component_state *c_st;
>>>>       struct komeda_component *c;
>>>> -    u32 disabling_comps, id;
>>>> +    u32 id;
>>>> +    unsigned long disabling_comps;
>>>>       WARN_ON(!old);
>>>> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
>>>> komeda_pipeline *pipe,
>>>>           st = komeda_pipeline_get_new_state(pipe, drm_st);
>>>>       else
>>>>           st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
>>>> NULL);
>>>> -
>>>
>>> NIT: Random white space change
>>>
>>>>       if (WARN_ON(IS_ERR_OR_NULL(st)))
>>>>           return -EINVAL;
>>>> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
>>>> komeda_pipeline *pipe,
>>>>       struct komeda_pipeline_state *old;
>>>>       struct komeda_component *c;
>>>>       struct komeda_component_state *c_st;
>>>> -    u32 id, disabling_comps = 0;
>>>> +    u32 id;
>>>> +    unsigned long disabling_comps;
>>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>>> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
>>>> komeda_pipeline *pipe,
>>>>           disabling_comps = old->active_comps &
>>>>                     pipe->standalone_disabled_comps;
>>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>>> 0x%x.\n",
>>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>>>> 0x%lx.\n",
>>>>                pipe->id, old->active_comps, disabling_comps);
>>>>       dp_for_each_set_bit(id, disabling_comps) {
>>>> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
>>>> komeda_pipeline *pipe,
>>>>       struct komeda_pipeline_state *new = 
>>>> priv_to_pipe_st(pipe->obj.state);
>>>>       struct komeda_pipeline_state *old;
>>>>       struct komeda_component *c;
>>>> -    u32 id, changed_comps = 0;
>>>> +    u32 id;
>>>> +    unsigned long changed_comps;
>>>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>>>       changed_comps = new->active_comps | old->active_comps;
>>>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
>>>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>>>>                pipe->id, new->active_comps, changed_comps);
>>>>       dp_for_each_set_bit(id, changed_comps) {
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Carsten Haitzler]

On 1/20/21 3:44 PM, Steven Price wrote:

Sent a new patch to list with updates as discussed.

> On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
>> From: Carsten Haitzler <carsten.haitzler@arm.com>
>>
>> Another issue found by KASAN. The bit finding is bueried inside the
> 
> NIT: s/bueried/buried/
> 
>> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
>> calls the bit stuff. These bit functions want an unsigned long pointer
>> as input and just dumbly casting leads to out-of-bounds accesses.
>> This fixes that.
> 
> This seems like an underlying bug/lack of clear documentation for the
> underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
> right thing:
> 
>    #define dp_for_each_set_bit(bit, mask) \
>        for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
> * 8)
> 
> i.e. passing the actual size of type. However because of the case to
> unsigned long (and subsequent accesses using that type) the compiler is
> free to make accesses beyond the size of the variable (and indeed this
> is completely broken on big-endian). The implementation actually
> requires that the bitmap is really an array of unsigned long - no other
> type will work.
> 
> So I think the fix should also include fixing the dp_for_each_set_bit()
> macro - the cast is bogus as it stands.
> 
> Doing that I also get warnings on komeda_pipeline::avail_comps and
> komeda_pipeline::supported_inputs - although I note there are other
> bitmasks mentioned.
> 
> The other option is to fix dp_for_each_set_bit() itself using a little 
> hack:
> 
> -       for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) 
> * 8)
> +       for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) 
> * 8)
> 
> With that I don't think you need any other change as the mask is actually
> in a new unsigned long on the stack.
> 
> Steve
> 
>>
>> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
>> ---
>>   .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git 
>> a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c 
>> b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> index e8b1e15312d8..f7dddb9f015d 100644
>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
>> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *old = 
>> priv_to_pipe_st(pipe->obj.state);
>>       struct komeda_component_state *c_st;
>>       struct komeda_component *c;
>> -    u32 disabling_comps, id;
>> +    u32 id;
>> +    unsigned long disabling_comps;
>>       WARN_ON(!old);
>> @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct 
>> komeda_pipeline *pipe,
>>           st = komeda_pipeline_get_new_state(pipe, drm_st);
>>       else
>>           st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, 
>> NULL);
>> -
> 
> NIT: Random white space change
> 
>>       if (WARN_ON(IS_ERR_OR_NULL(st)))
>>           return -EINVAL;
>> @@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *old;
>>       struct komeda_component *c;
>>       struct komeda_component_state *c_st;
>> -    u32 id, disabling_comps = 0;
>> +    u32 id;
>> +    unsigned long disabling_comps;
>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>> @@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct 
>> komeda_pipeline *pipe,
>>           disabling_comps = old->active_comps &
>>                     pipe->standalone_disabled_comps;
>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>> 0x%x.\n",
>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 
>> 0x%lx.\n",
>>                pipe->id, old->active_comps, disabling_comps);
>>       dp_for_each_set_bit(id, disabling_comps) {
>> @@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct 
>> komeda_pipeline *pipe,
>>       struct komeda_pipeline_state *new = 
>> priv_to_pipe_st(pipe->obj.state);
>>       struct komeda_pipeline_state *old;
>>       struct komeda_component *c;
>> -    u32 id, changed_comps = 0;
>> +    u32 id;
>> +    unsigned long changed_comps;
>>       old = komeda_pipeline_get_old_state(pipe, old_state);
>>       changed_comps = new->active_comps | old->active_comps;
>> -    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
>> +    DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>>                pipe->id, new->active_comps, changed_comps);
>>       dp_for_each_set_bit(id, changed_comps) {
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[James Qian Wang]

On Fri, Feb 05, 2021 at 08:29:32AM +0000, Steven Price wrote:
> +CC the other Komeda maintainers
> 
> On 04/02/2021 13:11, carsten.haitzler@foss.arm.com wrote:
> > From: Carsten Haitzler <carsten.haitzler@arm.com>
> > 
> > Another issue found by KASAN. The bit finding is buried inside the
> > dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
> > calls the bit stuff. These bit functions want an unsigned long pointer
> > as input and just dumbly casting leads to out-of-bounds accesses.
> > This fixes that.
> > 
> > Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Looks fine to me:
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
>

Thank you for the patch.

Reviewed-by: James Qian Wang <james.qian.wang@arm.com>

> > ---
> >   .../drm/arm/display/include/malidp_utils.h    |  3 ---
> >   .../drm/arm/display/komeda/komeda_pipeline.c  | 16 +++++++++++-----
> >   .../display/komeda/komeda_pipeline_state.c    | 19 +++++++++++--------
> >   3 files changed, 22 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> > index 3bc383d5bf73..49a1d7f3539c 100644
> > --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
> > +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> > @@ -13,9 +13,6 @@
> >   #define has_bit(nr, mask)	(BIT(nr) & (mask))
> >   #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
> > -#define dp_for_each_set_bit(bit, mask) \
> > -	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
> > -
> >   #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
> >   ({							\
> >   	int num_tries = __tries;			\
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> > index 719a79728e24..06c595378dda 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> > @@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
> >   {
> >   	struct komeda_component *c;
> >   	int i;
> > +	unsigned long avail_comps = pipe->avail_comps;
> > -	dp_for_each_set_bit(i, pipe->avail_comps) {
> > +	for_each_set_bit(i, &avail_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, i);
> >   		komeda_component_destroy(mdev, c);
> >   	}
> > @@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
> >   {
> >   	struct komeda_component *c;
> >   	int id;
> > +	unsigned long avail_comps = pipe->avail_comps;
> >   	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
> >   		 pipe->id, pipe->n_layers, pipe->n_scalers,
> > @@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
> >   		 pipe->of_output_links[1] ?
> >   		 pipe->of_output_links[1]->full_name : "none");
> > -	dp_for_each_set_bit(id, pipe->avail_comps) {
> > +	for_each_set_bit(id, &avail_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		komeda_component_dump(c);
> > @@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
> >   	struct komeda_pipeline *pipe = c->pipeline;
> >   	struct komeda_component *input;
> >   	int id;
> > +	unsigned long supported_inputs = c->supported_inputs;
> > -	dp_for_each_set_bit(id, c->supported_inputs) {
> > +	for_each_set_bit(id, &supported_inputs, 32) {
> >   		input = komeda_pipeline_get_component(pipe, id);
> >   		if (!input) {
> >   			c->supported_inputs &= ~(BIT(id));
> > @@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
> >   	struct komeda_component *c;
> >   	struct komeda_layer *layer;
> >   	int i, id;
> > +	unsigned long avail_comps = pipe->avail_comps;
> > -	dp_for_each_set_bit(id, pipe->avail_comps) {
> > +	for_each_set_bit(id, &avail_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		komeda_component_verify_inputs(c);
> >   	}
> > @@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
> >   {
> >   	struct komeda_component *c;
> >   	u32 id;
> > +	unsigned long avail_comps;
> >   	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
> >   	if (pipe->funcs && pipe->funcs->dump_register)
> >   		pipe->funcs->dump_register(pipe, sf);
> > -	dp_for_each_set_bit(id, pipe->avail_comps) {
> > +	avail_comps = pipe->avail_comps;
> > +	for_each_set_bit(id, &avail_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		seq_printf(sf, "\n------%s------\n", c->name);
> > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > index e8b1e15312d8..176cdf411f9f 100644
> > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> > @@ -1232,14 +1232,15 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
> >   	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
> >   	struct komeda_component_state *c_st;
> >   	struct komeda_component *c;
> > -	u32 disabling_comps, id;
> > +	u32 id;
> > +	unsigned long disabling_comps;
> >   	WARN_ON(!old);
> >   	disabling_comps = (~new->active_comps) & old->active_comps;
> >   	/* unbound all disabling component */
> > -	dp_for_each_set_bit(id, disabling_comps) {
> > +	for_each_set_bit(id, &disabling_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		c_st = komeda_component_get_state_and_set_user(c,
> >   				drm_st, NULL, new->crtc);
> > @@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
> >   	struct komeda_pipeline_state *old;
> >   	struct komeda_component *c;
> >   	struct komeda_component_state *c_st;
> > -	u32 id, disabling_comps = 0;
> > +	u32 id;
> > +	unsigned long disabling_comps;
> >   	old = komeda_pipeline_get_old_state(pipe, old_state);
> > @@ -1297,10 +1299,10 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
> >   		disabling_comps = old->active_comps &
> >   				  pipe->standalone_disabled_comps;
> > -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> > +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
> >   			 pipe->id, old->active_comps, disabling_comps);
> > -	dp_for_each_set_bit(id, disabling_comps) {
> > +	for_each_set_bit(id, &disabling_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		c_st = priv_to_comp_st(c->obj.state);
> > @@ -1331,16 +1333,17 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
> >   	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
> >   	struct komeda_pipeline_state *old;
> >   	struct komeda_component *c;
> > -	u32 id, changed_comps = 0;
> > +	u32 id;
> > +	unsigned long changed_comps;
> >   	old = komeda_pipeline_get_old_state(pipe, old_state);
> >   	changed_comps = new->active_comps | old->active_comps;
> > -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
> > +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
> >   			 pipe->id, new->active_comps, changed_comps);
> > -	dp_for_each_set_bit(id, changed_comps) {
> > +	for_each_set_bit(id, &changed_comps, 32) {
> >   		c = komeda_pipeline_get_component(pipe, id);
> >   		if (new->active_comps & BIT(c->id))
> > 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Steven Price]

+CC the other Komeda maintainers

On 04/02/2021 13:11, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Another issue found by KASAN. The bit finding is buried inside the
> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
> calls the bit stuff. These bit functions want an unsigned long pointer
> as input and just dumbly casting leads to out-of-bounds accesses.
> This fixes that.
> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>

Looks fine to me:

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>   .../drm/arm/display/include/malidp_utils.h    |  3 ---
>   .../drm/arm/display/komeda/komeda_pipeline.c  | 16 +++++++++++-----
>   .../display/komeda/komeda_pipeline_state.c    | 19 +++++++++++--------
>   3 files changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> index 3bc383d5bf73..49a1d7f3539c 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> @@ -13,9 +13,6 @@
>   #define has_bit(nr, mask)	(BIT(nr) & (mask))
>   #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
>   
> -#define dp_for_each_set_bit(bit, mask) \
> -	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
> -
>   #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
>   ({							\
>   	int num_tries = __tries;			\
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index 719a79728e24..06c595378dda 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
>   {
>   	struct komeda_component *c;
>   	int i;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(i, pipe->avail_comps) {
> +	for_each_set_bit(i, &avail_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, i);
>   		komeda_component_destroy(mdev, c);
>   	}
> @@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   {
>   	struct komeda_component *c;
>   	int id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
>   	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
>   		 pipe->id, pipe->n_layers, pipe->n_scalers,
> @@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   		 pipe->of_output_links[1] ?
>   		 pipe->of_output_links[1]->full_name : "none");
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	for_each_set_bit(id, &avail_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		komeda_component_dump(c);
> @@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
>   	struct komeda_pipeline *pipe = c->pipeline;
>   	struct komeda_component *input;
>   	int id;
> +	unsigned long supported_inputs = c->supported_inputs;
>   
> -	dp_for_each_set_bit(id, c->supported_inputs) {
> +	for_each_set_bit(id, &supported_inputs, 32) {
>   		input = komeda_pipeline_get_component(pipe, id);
>   		if (!input) {
>   			c->supported_inputs &= ~(BIT(id));
> @@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
>   	struct komeda_component *c;
>   	struct komeda_layer *layer;
>   	int i, id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	for_each_set_bit(id, &avail_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   		komeda_component_verify_inputs(c);
>   	}
> @@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
>   {
>   	struct komeda_component *c;
>   	u32 id;
> +	unsigned long avail_comps;
>   
>   	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
>   
>   	if (pipe->funcs && pipe->funcs->dump_register)
>   		pipe->funcs->dump_register(pipe, sf);
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	avail_comps = pipe->avail_comps;
> +	for_each_set_bit(id, &avail_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		seq_printf(sf, "\n------%s------\n", c->name);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index e8b1e15312d8..176cdf411f9f 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1232,14 +1232,15 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_component_state *c_st;
>   	struct komeda_component *c;
> -	u32 disabling_comps, id;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	WARN_ON(!old);
>   
>   	disabling_comps = (~new->active_comps) & old->active_comps;
>   
>   	/* unbound all disabling component */
> -	dp_for_each_set_bit(id, disabling_comps) {
> +	for_each_set_bit(id, &disabling_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   		c_st = komeda_component_get_state_and_set_user(c,
>   				drm_st, NULL, new->crtc);
> @@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
>   	struct komeda_component_state *c_st;
> -	u32 id, disabling_comps = 0;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
> @@ -1297,10 +1299,10 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   		disabling_comps = old->active_comps &
>   				  pipe->standalone_disabled_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
>   			 pipe->id, old->active_comps, disabling_comps);
>   
> -	dp_for_each_set_bit(id, disabling_comps) {
> +	for_each_set_bit(id, &disabling_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   		c_st = priv_to_comp_st(c->obj.state);
>   
> @@ -1331,16 +1333,17 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
> -	u32 id, changed_comps = 0;
> +	u32 id;
> +	unsigned long changed_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
>   	changed_comps = new->active_comps | old->active_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>   			 pipe->id, new->active_comps, changed_comps);
>   
> -	dp_for_each_set_bit(id, changed_comps) {
> +	for_each_set_bit(id, &changed_comps, 32) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		if (new->active_comps & BIT(c->id))
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 .../drm/arm/display/include/malidp_utils.h    |  3 ---
 .../drm/arm/display/komeda/komeda_pipeline.c  | 16 +++++++++++-----
 .../display/komeda/komeda_pipeline_state.c    | 19 +++++++++++--------
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..49a1d7f3539c 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -13,9 +13,6 @@
 #define has_bit(nr, mask)	(BIT(nr) & (mask))
 #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
 
-#define dp_for_each_set_bit(bit, mask) \
-	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
-
 #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
 ({							\
 	int num_tries = __tries;			\
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..06c595378dda 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
 {
 	struct komeda_component *c;
 	int i;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(i, pipe->avail_comps) {
+	for_each_set_bit(i, &avail_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, i);
 		komeda_component_destroy(mdev, c);
 	}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 {
 	struct komeda_component *c;
 	int id;
+	unsigned long avail_comps = pipe->avail_comps;
 
 	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
 		 pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 		 pipe->of_output_links[1] ?
 		 pipe->of_output_links[1]->full_name : "none");
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	for_each_set_bit(id, &avail_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
 	struct komeda_pipeline *pipe = c->pipeline;
 	struct komeda_component *input;
 	int id;
+	unsigned long supported_inputs = c->supported_inputs;
 
-	dp_for_each_set_bit(id, c->supported_inputs) {
+	for_each_set_bit(id, &supported_inputs, 32) {
 		input = komeda_pipeline_get_component(pipe, id);
 		if (!input) {
 			c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
 	struct komeda_component *c;
 	struct komeda_layer *layer;
 	int i, id;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	for_each_set_bit(id, &avail_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 		komeda_component_verify_inputs(c);
 	}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
 {
 	struct komeda_component *c;
 	u32 id;
+	unsigned long avail_comps;
 
 	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
 
 	if (pipe->funcs && pipe->funcs->dump_register)
 		pipe->funcs->dump_register(pipe, sf);
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	avail_comps = pipe->avail_comps;
+	for_each_set_bit(id, &avail_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		seq_printf(sf, "\n------%s------\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..176cdf411f9f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,14 +1232,15 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_component_state *c_st;
 	struct komeda_component *c;
-	u32 disabling_comps, id;
+	u32 id;
+	unsigned long disabling_comps;
 
 	WARN_ON(!old);
 
 	disabling_comps = (~new->active_comps) & old->active_comps;
 
 	/* unbound all disabling component */
-	dp_for_each_set_bit(id, disabling_comps) {
+	for_each_set_bit(id, &disabling_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 		c_st = komeda_component_get_state_and_set_user(c,
 				drm_st, NULL, new->crtc);
@@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
 	struct komeda_component_state *c_st;
-	u32 id, disabling_comps = 0;
+	u32 id;
+	unsigned long disabling_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
@@ -1297,10 +1299,10 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 		disabling_comps = old->active_comps &
 				  pipe->standalone_disabled_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
 			 pipe->id, old->active_comps, disabling_comps);
 
-	dp_for_each_set_bit(id, disabling_comps) {
+	for_each_set_bit(id, &disabling_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 		c_st = priv_to_comp_st(c->obj.state);
 
@@ -1331,16 +1333,17 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
-	u32 id, changed_comps = 0;
+	u32 id;
+	unsigned long changed_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
 	changed_comps = new->active_comps | old->active_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
 			 pipe->id, new->active_comps, changed_comps);
 
-	dp_for_each_set_bit(id, changed_comps) {
+	for_each_set_bit(id, &changed_comps, 32) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		if (new->active_comps & BIT(c->id))
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Steven Price]

NIT: This is the second version of this patch so should have "[PATCH 
v2]" in the subject.

On 27/01/2021 12:34, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Another issue found by KASAN. The bit finding is buried inside the
> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
> calls the bit stuff. These bit functions want an unsigned long pointer
> as input and just dumbly casting leads to out-of-bounds accesses.
> This fixes that.
> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
>   .../gpu/drm/arm/display/include/malidp_utils.h   | 10 ++++++++--
>   .../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++-----
>   .../arm/display/komeda/komeda_pipeline_state.c   | 13 ++++++++-----
>   3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> index 3bc383d5bf73..8d289cd0b5b8 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> @@ -12,9 +12,15 @@
>   
>   #define has_bit(nr, mask)	(BIT(nr) & (mask))
>   #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
> -
> +/*
> +#define dp_for_each_set_bit(bit, mask) \
> +	for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
> +#define dp_for_each_set_bit(bit, mask) \
> +	unsigned long __local_mask = mask; \
> +	for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
> +*/

Commented out code left in - please remove it.

>   #define dp_for_each_set_bit(bit, mask) \
> -	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
> +	for_each_set_bit((bit), &(mask), sizeof(mask) * 8)

I'm not really sure if there's much point in this macro now. In practice 
the uses below are now getting the wrong length (because sizeof(mask) == 
sizeof(unsigned long) ) but we actually know the size is smaller in most 
cases, so we could pass a more appropriate value in.

Other than that the changes below look correct to me.

Steve

>   
>   #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
>   ({							\
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index 719a79728e24..a85c8a806334 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
>   {
>   	struct komeda_component *c;
>   	int i;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(i, pipe->avail_comps) {
> +	dp_for_each_set_bit(i, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, i);
>   		komeda_component_destroy(mdev, c);
>   	}
> @@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   {
>   	struct komeda_component *c;
>   	int id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
>   	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
>   		 pipe->id, pipe->n_layers, pipe->n_scalers,
> @@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   		 pipe->of_output_links[1] ?
>   		 pipe->of_output_links[1]->full_name : "none");
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		komeda_component_dump(c);
> @@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
>   	struct komeda_pipeline *pipe = c->pipeline;
>   	struct komeda_component *input;
>   	int id;
> +	unsigned long supported_inputs = c->supported_inputs;
>   
> -	dp_for_each_set_bit(id, c->supported_inputs) {
> +	dp_for_each_set_bit(id, supported_inputs) {
>   		input = komeda_pipeline_get_component(pipe, id);
>   		if (!input) {
>   			c->supported_inputs &= ~(BIT(id));
> @@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
>   	struct komeda_component *c;
>   	struct komeda_layer *layer;
>   	int i, id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   		komeda_component_verify_inputs(c);
>   	}
> @@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
>   {
>   	struct komeda_component *c;
>   	u32 id;
> +	unsigned long avail_comps;
>   
>   	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
>   
>   	if (pipe->funcs && pipe->funcs->dump_register)
>   		pipe->funcs->dump_register(pipe, sf);
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	avail_comps = pipe->avail_comps;
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		seq_printf(sf, "\n------%s------\n", c->name);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index e8b1e15312d8..7640dae7f4bf 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_component_state *c_st;
>   	struct komeda_component *c;
> -	u32 disabling_comps, id;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	WARN_ON(!old);
>   
> @@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
>   	struct komeda_component_state *c_st;
> -	u32 id, disabling_comps = 0;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
> @@ -1297,7 +1299,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   		disabling_comps = old->active_comps &
>   				  pipe->standalone_disabled_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
>   			 pipe->id, old->active_comps, disabling_comps);
>   
>   	dp_for_each_set_bit(id, disabling_comps) {
> @@ -1331,13 +1333,14 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
> -	u32 id, changed_comps = 0;
> +	u32 id;
> +	unsigned long changed_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
>   	changed_comps = new->active_comps | old->active_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>   			 pipe->id, new->active_comps, changed_comps);
>   
>   	dp_for_each_set_bit(id, changed_comps) {
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 .../gpu/drm/arm/display/include/malidp_utils.h   | 10 ++++++++--
 .../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++-----
 .../arm/display/komeda/komeda_pipeline_state.c   | 13 ++++++++-----
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..8d289cd0b5b8 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -12,9 +12,15 @@
 
 #define has_bit(nr, mask)	(BIT(nr) & (mask))
 #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
-
+/*
+#define dp_for_each_set_bit(bit, mask) \
+	for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
+#define dp_for_each_set_bit(bit, mask) \
+	unsigned long __local_mask = mask; \
+	for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
+*/
 #define dp_for_each_set_bit(bit, mask) \
-	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
+	for_each_set_bit((bit), &(mask), sizeof(mask) * 8)
 
 #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
 ({							\
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..a85c8a806334 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
 {
 	struct komeda_component *c;
 	int i;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(i, pipe->avail_comps) {
+	dp_for_each_set_bit(i, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, i);
 		komeda_component_destroy(mdev, c);
 	}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 {
 	struct komeda_component *c;
 	int id;
+	unsigned long avail_comps = pipe->avail_comps;
 
 	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
 		 pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 		 pipe->of_output_links[1] ?
 		 pipe->of_output_links[1]->full_name : "none");
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
 	struct komeda_pipeline *pipe = c->pipeline;
 	struct komeda_component *input;
 	int id;
+	unsigned long supported_inputs = c->supported_inputs;
 
-	dp_for_each_set_bit(id, c->supported_inputs) {
+	dp_for_each_set_bit(id, supported_inputs) {
 		input = komeda_pipeline_get_component(pipe, id);
 		if (!input) {
 			c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
 	struct komeda_component *c;
 	struct komeda_layer *layer;
 	int i, id;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 		komeda_component_verify_inputs(c);
 	}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
 {
 	struct komeda_component *c;
 	u32 id;
+	unsigned long avail_comps;
 
 	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
 
 	if (pipe->funcs && pipe->funcs->dump_register)
 		pipe->funcs->dump_register(pipe, sf);
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	avail_comps = pipe->avail_comps;
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		seq_printf(sf, "\n------%s------\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..7640dae7f4bf 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_component_state *c_st;
 	struct komeda_component *c;
-	u32 disabling_comps, id;
+	u32 id;
+	unsigned long disabling_comps;
 
 	WARN_ON(!old);
 
@@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
 	struct komeda_component_state *c_st;
-	u32 id, disabling_comps = 0;
+	u32 id;
+	unsigned long disabling_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
@@ -1297,7 +1299,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 		disabling_comps = old->active_comps &
 				  pipe->standalone_disabled_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
 			 pipe->id, old->active_comps, disabling_comps);
 
 	dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1333,14 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
-	u32 id, changed_comps = 0;
+	u32 id;
+	unsigned long changed_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
 	changed_comps = new->active_comps | old->active_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
 			 pipe->id, new->active_comps, changed_comps);
 
 	dp_for_each_set_bit(id, changed_comps) {
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type

[Liviu Dudau]

On Fri, Dec 18, 2020 at 03:08:12PM +0000, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> KASAN found this problem. find_first_bit() expects to look at a
> pointer pointing to a long, but we look at a u32 - this is going to be
> an issue with endianess but, KSAN already flags this as out-of-bounds
> stack reads. This fixes it by just importing inot a local long.
> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index 452e505a1fd3..719a79728e24 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -137,9 +137,10 @@ komeda_pipeline_get_first_component(struct komeda_pipeline *pipe,
>  				    u32 comp_mask)
>  {
>  	struct komeda_component *c = NULL;
> +	unsigned long comp_mask_local = (unsigned long)comp_mask;
>  	int id;
>  
> -	id = find_first_bit((unsigned long *)&comp_mask, 32);
> +	id = find_first_bit(&comp_mask_local, 32);
>  	if (id < 32)
>  		c = komeda_pipeline_get_component(pipe, id);
>  
> -- 
> 2.29.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/komeda: Fix bit check to import to value of proper type

[carsten.haitzler]

From: Carsten Haitzler <carsten.haitzler@arm.com>

KASAN found this problem. find_first_bit() expects to look at a
pointer pointing to a long, but we look at a u32 - this is going to be
an issue with endianess but, KSAN already flags this as out-of-bounds
stack reads. This fixes it by just importing inot a local long.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 452e505a1fd3..719a79728e24 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -137,9 +137,10 @@ komeda_pipeline_get_first_component(struct komeda_pipeline *pipe,
 				    u32 comp_mask)
 {
 	struct komeda_component *c = NULL;
+	unsigned long comp_mask_local = (unsigned long)comp_mask;
 	int id;
 
-	id = find_first_bit((unsigned long *)&comp_mask, 32);
+	id = find_first_bit(&comp_mask_local, 32);
 	if (id < 32)
 		c = komeda_pipeline_get_component(pipe, id);
 
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel