| Message ID | 20231206072701.13276-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/plane: fix error handling in __drm_universal_plane_init | expand |
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.) 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; > }
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)
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) > > >
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; }
__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> --- drivers/gpu/drm/drm_plane.c | 3 +++ 1 file changed, 3 insertions(+)