[PATCH] drm/komeda: Fix bit check to import to value of proper type
Carsten Haitzler
carsten.haitzler at foss.arm.com
Thu Jan 21 17:37:40 UTC 2021
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 at foss.arm.com wrote:
>>>> From: Carsten Haitzler <carsten.haitzler at 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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
More information about the dri-devel
mailing list