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

From: Carsten Haitzler <carsten.haitzler@foss.arm.com>
To: Steven Price <steven.price@arm.com>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
Date: Thu, 21 Jan 2021 17:37:40 +0000	[thread overview]
Message-ID: <ea42e94a-5633-5d34-19a1-876ed90db67d@foss.arm.com> (raw)
In-Reply-To: <8151e649-7b9c-c13c-a36d-d2b08e1e608d@arm.com>

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