Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231206072701.13276-1-dinghao.liu@zju.edu.cn> <77925c2e-8cb2-473b-9dd0-3ce60a909066@suse.de>
In-Reply-To: <77925c2e-8cb2-473b-9dd0-3ce60a909066@suse.de>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Wed, 6 Dec 2023 11:09:25 +0200
Message-ID: <CAA8EJprUDgraGgiGKsh0PSVd6Zxe9L2Q+HTPrALHWFVP3hiq1Q@mail.gmail.com>
Subject: Re: [PATCH] drm/plane: fix error handling in __drm_universal_plane_init
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Dinghao Liu <dinghao.liu@zju.edu.cn>,
        Maxime Ripard <mripard@kernel.org>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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