[PATCH] drm/komeda: Fix bit check to import to value of proper type
Steven Price
steven.price at arm.com
Thu Jan 21 16:40:08 UTC 2021
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...
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