__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 <[email protected]>
---
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;
}
--
2.17.1
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 <[email protected]>
Reviewed-by: Thomas Zimmermann <[email protected]>
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.)
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)
On Wed, 6 Dec 2023 at 10:22, Thomas Zimmermann <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Thomas Zimmermann <[email protected]>
>
> 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
Hi
Am 06.12.23 um 10:09 schrieb Dmitry Baryshkov:
> On Wed, 6 Dec 2023 at 10:22, Thomas Zimmermann <[email protected]> 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 <[email protected]>
>>
>> Reviewed-by: Thomas Zimmermann <[email protected]>
>>
>> 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)