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
Cc: liviu.dudau@arm.com, Carsten Haitzler <carsten.haitzler@arm.com>
Subject: Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
Date: Wed, 27 Jan 2021 12:35:47 +0000	[thread overview]
Message-ID: <8511796e-0526-7fa9-9058-f2fddc825f27@foss.arm.com> (raw)
In-Reply-To: <400e2a79-aae6-33d7-4587-21e3c96ccf04@arm.com>

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