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, > - <dc_plane_funcs, formats, nb_fmt, > - modifiers, type, NULL); > - if (ret < 0) > + plane = drmm_universal_plane_alloc(ddev, struct drm_plane, dev, > + possible_crtcs, <dc_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, > <dc_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, > <dc_crtc_funcs, NULL); > if (ret) { > DRM_ERROR("Can not initialize CRTC\n"); > - goto cleanup; > + return ret; > } > > drm_crtc_helper_add(crtc, <dc_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, <dc_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); > } >
