Re: [PATCH v2] drm/stm: Avoid use-after-free issues with crtc and plane

From: Raphael Gallais-Pou
Date: Fri Jan 05 2024 - 04:22:01 EST


On 11/24/23 11:04, Katya Orlova wrote:
> ltdc_load() calls functions drm_crtc_init_with_planes(),
> drm_universal_plane_init() and drm_encoder_init(). These functions
> should not be called with parameters allocated with devm_kzalloc()
> to avoid use-after-free issues [1].
>
> Use allocations managed by the DRM framework.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> [1]
> https://lore.kernel.org/lkml/u366i76e3qhh3ra5oxrtngjtm2u5lterkekcz6y2jkndhuxzli@diujon4h7qwb/
>
> Signed-off-by: Katya Orlova <e.orlova@xxxxxxxxx>
> ---
> v2: use allocations managed by the DRM as
> Raphael Gallais-Pou <raphael.gallais-pou@xxxxxxxxxxx> suggested.
> Also add a fix for encoder.
> drivers/gpu/drm/stm/drv.c | 3 +-
> drivers/gpu/drm/stm/ltdc.c | 68 +++++++++-----------------------------
> 2 files changed, 18 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index e8523abef27a..152bec2c0238 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -25,6 +25,7 @@
> #include <drm/drm_module.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>
>
> #include "ltdc.h"
>
> @@ -75,7 +76,7 @@ static int drv_load(struct drm_device *ddev)
>
> DRM_DEBUG("%s\n", __func__);
>
> - ldev = devm_kzalloc(ddev->dev, sizeof(*ldev), GFP_KERNEL);
> + ldev = drmm_kzalloc(ddev, sizeof(*ldev), GFP_KERNEL);
> if (!ldev)
> return -ENOMEM;
>
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 5576fdae4962..02a7c8375f44 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -36,6 +36,7 @@
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>
>
> #include <video/videomode.h>
>
> @@ -1199,7 +1200,6 @@ static void ltdc_crtc_atomic_print_state(struct drm_printer *p,
> }
>
> static const struct drm_crtc_funcs ltdc_crtc_funcs = {
> - .destroy = drm_crtc_cleanup,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .reset = drm_atomic_helper_crtc_reset,
> @@ -1212,7 +1212,6 @@ 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,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .reset = drm_atomic_helper_crtc_reset,
> @@ -1545,7 +1544,6 @@ static void ltdc_plane_atomic_print_state(struct drm_printer *p,
> 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,
> .reset = drm_atomic_helper_plane_reset,
> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> @@ -1572,7 +1570,6 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
> const u64 *modifiers = ltdc_format_modifiers;
> u32 lofs = index * LAY_OFS;
> u32 val;
> - int ret;
>
> /* Allocate the biggest size according to supported color formats */
> formats = devm_kzalloc(dev, (ldev->caps.pix_fmt_nb +
> @@ -1613,14 +1610,10 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
> }
> }
>
> - plane = devm_kzalloc(dev, sizeof(*plane), GFP_KERNEL);
> - if (!plane)
> - return NULL;
> -
> - ret = drm_universal_plane_init(ddev, plane, possible_crtcs,
> - &ltdc_plane_funcs, formats, nb_fmt,
> - modifiers, type, NULL);
> - if (ret < 0)
> + plane = drmm_universal_plane_alloc(ddev, struct drm_plane, dev,
> + possible_crtcs, &ltdc_plane_funcs, formats, nb_fmt,
> + modifiers, type, NULL);

Hi Katya,

Thanks for your submission, and sorry for the delay.


There is several alignment style problems, such as the lines above.

You can use "--strict" option with checkpatch script to show you all the faulty
alignment before sending a patch.


Other than that this patch looks pretty good to me.

Regards,

Raphaël

> + if (IS_ERR(plane))
> return NULL;
>
> if (ldev->caps.ycbcr_input) {
> @@ -1643,15 +1636,6 @@ static struct drm_plane *ltdc_plane_create(struct drm_device *ddev,
> return plane;
> }
>
> -static void ltdc_plane_destroy_all(struct drm_device *ddev)
> -{
> - struct drm_plane *plane, *plane_temp;
> -
> - list_for_each_entry_safe(plane, plane_temp,
> - &ddev->mode_config.plane_list, head)
> - drm_plane_cleanup(plane);
> -}
> -
> static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
> {
> struct ltdc_device *ldev = ddev->dev_private;
> @@ -1677,14 +1661,14 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>
> /* Init CRTC according to its hardware features */
> if (ldev->caps.crc)
> - ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> + ret = drmm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> &ltdc_crtc_with_crc_support_funcs, NULL);
> else
> - ret = drm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> + ret = drmm_crtc_init_with_planes(ddev, crtc, primary, NULL,
> &ltdc_crtc_funcs, NULL);
> if (ret) {
> DRM_ERROR("Can not initialize CRTC\n");
> - goto cleanup;
> + return ret;
> }
>
> drm_crtc_helper_add(crtc, &ltdc_crtc_helper_funcs);
> @@ -1698,9 +1682,8 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
> for (i = 1; i < ldev->caps.nb_layers; i++) {
> overlay = ltdc_plane_create(ddev, DRM_PLANE_TYPE_OVERLAY, i);
> if (!overlay) {
> - ret = -ENOMEM;
> DRM_ERROR("Can not create overlay plane %d\n", i);
> - goto cleanup;
> + return -ENOMEM;
> }
> if (ldev->caps.dynamic_zorder)
> drm_plane_create_zpos_property(overlay, i, 0, ldev->caps.nb_layers - 1);
> @@ -1713,10 +1696,6 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
> }
>
> return 0;
> -
> -cleanup:
> - ltdc_plane_destroy_all(ddev);
> - return ret;
> }
>
> static void ltdc_encoder_disable(struct drm_encoder *encoder)
> @@ -1776,23 +1755,19 @@ static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge *bridge)
> struct drm_encoder *encoder;
> int ret;
>
> - encoder = devm_kzalloc(ddev->dev, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder)
> - return -ENOMEM;
> + encoder = drmm_simple_encoder_alloc(ddev, struct drm_encoder, dev,
> + DRM_MODE_ENCODER_DPI);
Nit: bad alignment.
> + if (IS_ERR(encoder))
> + return PTR_ERR(encoder);
>
> encoder->possible_crtcs = CRTC_MASK;
> encoder->possible_clones = 0; /* No cloning support */
>
> - drm_simple_encoder_init(ddev, encoder, DRM_MODE_ENCODER_DPI);
> -
> drm_encoder_helper_add(encoder, &ltdc_encoder_helper_funcs);
>
> ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> - if (ret) {
> - if (ret != -EPROBE_DEFER)
> - drm_encoder_cleanup(encoder);
> + if (ret)
> return ret;
> - }
>
> DRM_DEBUG_DRIVER("Bridge encoder:%d created\n", encoder->base.id);
>
> @@ -1962,8 +1937,7 @@ int ltdc_load(struct drm_device *ddev)
> goto err;
>
> if (panel) {
> - bridge = drm_panel_bridge_add_typed(panel,
> - DRM_MODE_CONNECTOR_DPI);
> + bridge = drmm_panel_bridge_add(ddev, panel);
> if (IS_ERR(bridge)) {
> DRM_ERROR("panel-bridge endpoint %d\n", i);
> ret = PTR_ERR(bridge);
> @@ -2045,7 +2019,7 @@ int ltdc_load(struct drm_device *ddev)
> }
> }
>
> - crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
> + crtc = drmm_kzalloc(ddev, sizeof(*crtc), GFP_KERNEL);
> if (!crtc) {
> DRM_ERROR("Failed to allocate crtc\n");
> ret = -ENOMEM;
> @@ -2072,8 +2046,6 @@ int ltdc_load(struct drm_device *ddev)
>
> return 0;
> err:
> - for (i = 0; i < nb_endpoints; i++)
> - drm_of_panel_bridge_remove(ddev->dev->of_node, 0, i);
>
> clk_disable_unprepare(ldev->pixel_clk);
>
> @@ -2082,16 +2054,8 @@ int ltdc_load(struct drm_device *ddev)
>
> void ltdc_unload(struct drm_device *ddev)
> {
> - struct device *dev = ddev->dev;
> - int nb_endpoints, i;
> -
> DRM_DEBUG_DRIVER("\n");
>
> - nb_endpoints = of_graph_get_endpoint_count(dev->of_node);
> -
> - for (i = 0; i < nb_endpoints; i++)
> - drm_of_panel_bridge_remove(ddev->dev->of_node, 0, i);
> -
> pm_runtime_disable(ddev->dev);
> }
>