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. > > 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) -- With best wishes Dmitry
