Re: [PATCH] drm/plane: fix error handling in __drm_universal_plane_init

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3768 bytes --]

Hi

Am 06.12.23 um 10:09 schrieb Dmitry Baryshkov:
> On Wed, 6 Dec 2023 at 10:22, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 06.12.23 um 08:27 schrieb Dinghao Liu:
>>> __drm_universal_plane_init() frees plane->format_types and
>>> plane->modifiers on failure. However, sometimes its callers
>>> will free these two pointers again, which may lead to a
>>> double-free. One possible call chain is:
>>>
>>> mdp5_plane_init
>>>     |-> drm_universal_plane_init
>>>     |     |-> __drm_universal_plane_init (first free)
>>>     |
>>>     |-> mdp5_plane_destroy
>>>           |-> drm_plane_cleanup (second free)
>>>
>>> Fix this by setting the two pointers to NULL after kfree.
>>>
>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> to get the immediate bug fixed.
>>
>> However, I don't think it's the correct way of doing things in general.
>> Planes should probably not attempt to even make a copy, but use the
>> supplied pointers. Lifetime of the arrays is the same as of the plane.
>> That's for a different patch set, of course.  (I did not review the DRM
>> code whether the internal copy is required.)
> 
> But there is no internal copy. The issue is in the mdp5 code calling
> drm_plane_cleanup (indirectly) even though the plane init has failed.

I know. It calls drm_plane_cleanup() even though _plane_init() didn't 
succeed. It should really just free the plane's allocated memory.

The internal copying I'm referring to is in 
__drm_universal_plane_init(). I'm not 100% sure, but it seems 
unnecessary to me. Most drivers declare their format and modifier arrays 
as static const. No need to copy that.

Best regards
Thomas

> 
>>
>> For now, maybe drm_plane_cleanup() could warn if format_types equals
>> NULL. [1] It indicates that the plane has not been initialized correctly
>> and the driver's memory lifetime handling is somehow broken.
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_plane.c#L542
>>
>>> ---
>>>    drivers/gpu/drm/drm_plane.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>>> index 24e7998d1731..1331b8224920 100644
>>> --- a/drivers/gpu/drm/drm_plane.c
>>> +++ b/drivers/gpu/drm/drm_plane.c
>>> @@ -304,6 +304,7 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>>>        if (format_modifier_count && !plane->modifiers) {
>>>                DRM_DEBUG_KMS("out of memory when allocating plane\n");
>>>                kfree(plane->format_types);
>>> +             plane->format_types = NULL;
>>>                drm_mode_object_unregister(dev, &plane->base);
>>>                return -ENOMEM;
>>>        }
>>> @@ -317,6 +318,8 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>>>        if (!plane->name) {
>>>                kfree(plane->format_types);
>>>                kfree(plane->modifiers);
>>> +             plane->format_types = NULL;
>>> +             plane->modifiers = NULL;
>>>                drm_mode_object_unregister(dev, &plane->base);
>>>                return -ENOMEM;
>>>        }
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]