Hi Am 06.12.23 um 10:09 schrieb Dmitry Baryshkov:
On Wed, 6 Dec 2023 at 10:22, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxx>Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx> 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:
OpenPGP_signature.asc
Description: OpenPGP digital signature
