Re: [PATCH] drm/stm: Avoid use-after-free issues with crtc and plane
Hello Katya,
Thanks for your submission.
Please see drivers/gpu/drm/cr
On 10/20/23 12:29, Katya Orlova wrote:
> ltdc_load() calls functions drm_crtc_init_with_planes() and
> drm_universal_plane_init(). These functions should not be called with
> parameters allocated with devm_kzalloc() to avoid use-after-free issues [1].
What about drmm_kzalloc() ? By doing so the drm device is properly destroyed
using DRM internals, rather than handling the allocation within the LTDC driver.
This way has two benefits IMHO : you don't have to implement the .destroy hook
for CRTC and planes and since the allocation is managed by the DRM framework it
is less likely to make resources allocation errors on future patches.
You would then have to use drmm_universal_plane_init() and
drmm_crtc_init_with_planes().
see include/drm/drm_plane.h:779 and drivers/gpu/drm/drm_crtc.c:409
Regards,
Raphaël Gallais-Pou
>
> The patch replaces managed resource allocations with regular ones.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> [1]
> https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/
>
> Signed-off-by: Katya Orlova
> ---
> drivers/gpu/drm/stm/drv.c | 11 --
> drivers/gpu/drm/stm/ltdc.c | 72 ++
> 2 files changed, 58 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index cb4404b3ce62..409f26d3e400 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -74,7 +74,7 @@ static int drv_load(struct drm_device *ddev)
>
> DRM_DEBUG("%s\n", __func__);
>
> - ldev = devm_kzalloc(ddev->dev, sizeof(*ldev), GFP_KERNEL);
> + ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
> if (!ldev)
> return -ENOMEM;
>
> @@ -82,7 +82,7 @@ static int drv_load(struct drm_device *ddev)
>
> ret = drmm_mode_config_init(ddev);
> if (ret)
> - return ret;
> + goto err;
>
> /*
>* set max width and height as default value.
> @@ -98,7 +98,7 @@ static int drv_load(struct drm_device *ddev)
>
> ret = ltdc_load(ddev);
> if (ret)
> - return ret;
> + goto err;
>
> drm_mode_config_reset(ddev);
> drm_kms_helper_poll_init(ddev);
> @@ -106,6 +106,9 @@ static int drv_load(struct drm_device *ddev)
> platform_set_drvdata(pdev, ddev);
>
> return 0;
> +err:
> + kfree(ldev);
> + return ret;
> }
>
> static void drv_unload(struct drm_device *ddev)
> @@ -114,6 +117,8 @@ static void drv_unload(struct drm_device *ddev)
>
> drm_kms_helper_poll_fini(ddev);
> ltdc_unload(ddev);
> + kfree(ddev->dev_private);
> + ddev->dev_private = NULL;
> }
>
> static __maybe_unused int drv_suspend(struct device *dev)
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index b8be4c1db423..ec3bc3637a63 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -1120,6 +1120,12 @@ static const struct drm_crtc_helper_funcs
> ltdc_crtc_helper_funcs = {
> .get_scanout_position = ltdc_crtc_get_scanout_position,
> };
>
> +static void ltdc_crtc_destroy(struct drm_crtc *crtc)
> +{
> + drm_crtc_cleanup(crtc);
> + kfree(crtc);
> +}
> +
> static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
> {
> struct ltdc_device *ldev = crtc_to_ltdc(crtc);
> @@ -1200,7 +1206,7 @@ static void ltdc_crtc_atomic_print_state(struct
> drm_printer *p,
> }
>
> static const struct drm_crtc_funcs ltdc_crtc_funcs = {
> - .destroy = drm_crtc_cleanup,
> + .destroy = ltdc_crtc_destroy,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .reset = drm_atomic_helper_crtc_reset,
> @@ -1213,7 +1219,7 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
> };
>
> static const struct drm_crtc_funcs ltdc_crtc_with_crc_support_funcs = {
> - .destroy = drm_crtc_cleanup,
> + .destroy = ltdc_crtc_destroy,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .reset = drm_atomic_helper_crtc_reset,
> @@ -1543,10 +1549,16 @@ static void ltdc_plane_atomic_print_state(struct
> drm_printer *p,
> fpsi->counter = 0;
> }
>
> +static void ltdc_plane_destroy(struct drm_plane *plane)
> +{
> + drm_plane_cleanup(plane);
> + kfree(plane);
> +}
> +
> static const struct drm_plane_funcs ltdc_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> - .destroy = drm_plane_cleanup,
> + .destroy = ltdc_plane_destroy,
> .reset = drm_atomic_helper_plane_reset,
> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> @@ -1565,7 +1577,6 @@ static struct drm_plane *ltdc_plane_create(struct
> drm_device
[PATCH] drm/stm: Avoid use-after-free issues with crtc and plane
ltdc_load() calls functions drm_crtc_init_with_planes() and
drm_universal_plane_init(). These functions should not be called with
parameters allocated with devm_kzalloc() to avoid use-after-free issues [1].
The patch replaces managed resource allocations with regular ones.
Found by Linux Verification Center (linuxtesting.org).
[1]
https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/
Signed-off-by: Katya Orlova
---
drivers/gpu/drm/stm/drv.c | 11 --
drivers/gpu/drm/stm/ltdc.c | 72 ++
2 files changed, 58 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index cb4404b3ce62..409f26d3e400 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -74,7 +74,7 @@ static int drv_load(struct drm_device *ddev)
DRM_DEBUG("%s\n", __func__);
- ldev = devm_kzalloc(ddev->dev, sizeof(*ldev), GFP_KERNEL);
+ ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);
if (!ldev)
return -ENOMEM;
@@ -82,7 +82,7 @@ static int drv_load(struct drm_device *ddev)
ret = drmm_mode_config_init(ddev);
if (ret)
- return ret;
+ goto err;
/*
* set max width and height as default value.
@@ -98,7 +98,7 @@ static int drv_load(struct drm_device *ddev)
ret = ltdc_load(ddev);
if (ret)
- return ret;
+ goto err;
drm_mode_config_reset(ddev);
drm_kms_helper_poll_init(ddev);
@@ -106,6 +106,9 @@ static int drv_load(struct drm_device *ddev)
platform_set_drvdata(pdev, ddev);
return 0;
+err:
+ kfree(ldev);
+ return ret;
}
static void drv_unload(struct drm_device *ddev)
@@ -114,6 +117,8 @@ static void drv_unload(struct drm_device *ddev)
drm_kms_helper_poll_fini(ddev);
ltdc_unload(ddev);
+ kfree(ddev->dev_private);
+ ddev->dev_private = NULL;
}
static __maybe_unused int drv_suspend(struct device *dev)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index b8be4c1db423..ec3bc3637a63 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -1120,6 +1120,12 @@ static const struct drm_crtc_helper_funcs
ltdc_crtc_helper_funcs = {
.get_scanout_position = ltdc_crtc_get_scanout_position,
};
+static void ltdc_crtc_destroy(struct drm_crtc *crtc)
+{
+ drm_crtc_cleanup(crtc);
+ kfree(crtc);
+}
+
static int ltdc_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct ltdc_device *ldev = crtc_to_ltdc(crtc);
@@ -1200,7 +1206,7 @@ static void ltdc_crtc_atomic_print_state(struct
drm_printer *p,
}
static const struct drm_crtc_funcs ltdc_crtc_funcs = {
- .destroy = drm_crtc_cleanup,
+ .destroy = ltdc_crtc_destroy,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
.reset = drm_atomic_helper_crtc_reset,
@@ -1213,7 +1219,7 @@ static const struct drm_crtc_funcs ltdc_crtc_funcs = {
};
static const struct drm_crtc_funcs ltdc_crtc_with_crc_support_funcs = {
- .destroy = drm_crtc_cleanup,
+ .destroy = ltdc_crtc_destroy,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
.reset = drm_atomic_helper_crtc_reset,
@@ -1543,10 +1549,16 @@ static void ltdc_plane_atomic_print_state(struct
drm_printer *p,
fpsi->counter = 0;
}
+static void ltdc_plane_destroy(struct drm_plane *plane)
+{
+ drm_plane_cleanup(plane);
+ kfree(plane);
+}
+
static const struct drm_plane_funcs ltdc_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .destroy = drm_plane_cleanup,
+ .destroy = ltdc_plane_destroy,
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
@@ -1565,7 +1577,6 @@ static struct drm_plane *ltdc_plane_create(struct
drm_device *ddev,
{
unsigned long possible_crtcs = CRTC_MASK;
struct ltdc_device *ldev = ddev->dev_private;
- struct device *dev = ddev->dev;
struct drm_plane *plane;
unsigned int i, nb_fmt = 0;
u32 *formats;
@@ -1576,7 +1587,7 @@ static struct drm_plane *ltdc_plane_create(struct
drm_device *ddev,
int ret;
/* Allocate the biggest size according to supported color formats */
- formats = devm_kzalloc(dev, (ldev->caps.pix_fmt_nb +
+ formats = kzalloc((ldev->caps.pix_fmt_nb +
ARRAY_SIZE(ltdc_drm_fmt_ycbcr_cp) +
ARRAY_SIZE(ltdc_drm_fmt_ycbcr_sp) +
ARRAY_SIZE(ltdc_drm_fmt_ycbcr_fp)) *
@@ -1614,15 +1625,20 @@ static struct drm_plane *ltdc_plane_create(struct
drm_device *ddev,
