Re: [PATCH] drm/komeda: Fix an undefined behavior bug in komeda_plane_add()

[Steven Price <steven.price@arm.com>]

On 01/12/2021 21:15, Liviu Dudau wrote:
> On Wed, Dec 01, 2021 at 03:44:03PM +0000, Steven Price wrote:
>> On 30/11/2021 14:25, Zhou Qingyang wrote:
>>> In komeda_plane_add(), komeda_get_layer_fourcc_list() is assigned to
>>> formats and used in drm_universal_plane_init().
>>> drm_universal_plane_init() passes formats to
>>> __drm_universal_plane_init(). __drm_universal_plane_init() further
>>> passes formats to memcpy() as src parameter, which could lead to an
>>> undefined behavior bug on failure of komeda_get_layer_fourcc_list().
>>>
>>> Fix this bug by adding a check of formats.
>>>
>>> This bug was found by a static analyzer. The analysis employs
>>> differential checking to identify inconsistent security operations
>>> (e.g., checks or kfrees) between two code paths and confirms that the
>>> inconsistent operations are not recovered in the current function or
>>> the callers, so they constitute bugs.
>>>
>>> Note that, as a bug found by static analysis, it can be a false
>>> positive or hard to trigger. Multiple researchers have cross-reviewed
>>> the bug.
>>>
>>> Builds with CONFIG_DRM_KOMEDA=m show no new warnings,
>>> and our static analyzer no longer warns about this code.
>>>
>>> Fixes: 61f1c4a8ab75 ("drm/komeda: Attach komeda_dev to DRM-KMS")
>>> Signed-off-by: Zhou Qingyang <zhou1615@umn.edu>
>>> ---
>>>  drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
>>> index d63d83800a8a..dd3f17e970dd 100644
>>> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
>>> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
>>> @@ -265,6 +265,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>>>  
>>>  	formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
>>>  					       layer->layer_type, &n_formats);
>>> +	if (!formats) {
>>> +		err = -ENOMEM;
>>> +		goto cleanup;
>>> +	}
>>
>> If this executes it will cause undefined behaviour...
>>
>> The cleanup code calls komeda_plane_destroy() which calls
>> drm_plane_cleanup() which does (amongst other things) a
>> list_del(&plane->head). But the plane hasn't been put on a list yet as
>> that's done in drm_universal_plane_init().
>>
>> So in this case we simple want to do:
>>
>> if (!formats) {
>> 	kfree(kplane);
>> 	return -ENOMEM;
>> }
> 
> Zhou has already posted v2 that contains this fix.

Sorry, for some reason Zhou's patch appeared twice on the list and I
hadn't spotted your reply to the other version. My mistake.

>>
>> Note that without this 'fix' a NULL return from
>> komeda_get_layer_fourcc_list() would leave n_formats==0, so while the
>> NULL pointer is passed into memcpy() it is also passed a length of 0.
>> Which I believe is safe.
>>
>> However while looking at this function...
>>
>>>  
>>>  	err = drm_universal_plane_init(&kms->base, plane,
>>>  			get_possible_crtcs(kms, c->pipeline),
>>>
>>
>> This call to drm_universal_plane_init() can fail early before
>> plane->head has been initialised. In which case the following:
>>
>>> 	komeda_put_fourcc_list(formats);
>>>
>>> 	if (err)
>>> 		goto cleanup;
>>
>> commits the exact same sin and would cause a similar NULL dereference in
>> drm_plane_cleanup().
> 
> I will come up with a patch for this case and post it to the list tomorrow.

Great, thanks for taking a look - I'm afraid I couldn't see an obvious fix.

Regards,

Steven