linux-kernel - Re: [PATCH] drm/komeda: Fix an undefined behavior bug in komeda_plane

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <95728378-3af0-2b55-0545-46d5131fa491@arm.com>
Date:   Thu, 2 Dec 2021 09:39:58 +0000
From:   Steven Price <steven.price@....com>
To:     Liviu Dudau <liviu.dudau@....com>
Cc:     Zhou Qingyang <zhou1615@....edu>, David Airlie <airlied@...ux.ie>,
        kjlu@....edu, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        "James (Qian) Wang" <james.qian.wang@....com>,
        Mihail Atanassov <mihail.atanassov@....com>
Subject: Re: [PATCH] drm/komeda: Fix an undefined behavior bug in
 komeda_plane_add()

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@....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