Hi Tomi,
Thank you for the patch.
On Wed, Apr 15, 2020 at 12:20:06PM +0300, Tomi Valkeinen wrote:
> tidss uses devm_kzalloc to allocate DRM plane, encoder and crtc objects.
> This is not correct as the lifetime of those objects should be longer
> than the underlying device's.
>
> When unloading tidss module, the devm_kzalloc'ed objects have already
> been freed when tidss_release() is called, and the driver will accesses
> freed memory possibly causing a crash, a kernel WARN, or other undefined
> behavior, and also KASAN will give a bug.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> ---
> drivers/gpu/drm/tidss/tidss_crtc.c | 16 +++++++++++++---
> drivers/gpu/drm/tidss/tidss_encoder.c | 14 +++++++++++---
> drivers/gpu/drm/tidss/tidss_plane.c | 24 ++++++++++++++++++------
> 3 files changed, 42 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index d4ce9bab8c7e..3221a707e073 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -379,9 +379,17 @@ static struct drm_crtc_state *tidss_crtc_duplicate_state(struct drm_crtc *crtc)
> return &state->base;
> }
>
> +static void tidss_crtc_destroy(struct drm_crtc *crtc)
> +{
> + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> +
> + drm_crtc_cleanup(crtc);
> + kfree(tcrtc);
I would personally store the CRTC pointers, or embed the CRTC instances
in the tidss_device structure, and free everything in the top-level
tidss_release() handler, to avoid spreading the release code all around
the driver. Same for planes and encoders. It may be a matter of personal
taste though, but it would allow dropping the kfree() calls in
individual error paths and centralize them in a single place if you
store the allocated pointer in tidss_device right after allocation.
> +}
> +
> static const struct drm_crtc_funcs tidss_crtc_funcs = {
> .reset = tidss_crtc_reset,
> - .destroy = drm_crtc_cleanup,
> + .destroy = tidss_crtc_destroy,
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> .atomic_duplicate_state = tidss_crtc_duplicate_state,
> @@ -400,7 +408,7 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
> bool has_ctm = tidss->feat->vp_feat.color.has_ctm;
> int ret;
>
> - tcrtc = devm_kzalloc(tidss->dev, sizeof(*tcrtc), GFP_KERNEL);
> + tcrtc = kzalloc(sizeof(*tcrtc), GFP_KERNEL);
> if (!tcrtc)
> return ERR_PTR(-ENOMEM);
>
> @@ -411,8 +419,10 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
>
> ret = drm_crtc_init_with_planes(&tidss->ddev, crtc, primary,
> NULL, &tidss_crtc_funcs, NULL);
> - if (ret < 0)
> + if (ret < 0) {
> + kfree(tcrtc);
> return ERR_PTR(ret);
> + }
>
> drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
>
> diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
> index 83785b0a66a9..30bf2a65949c 100644
> --- a/drivers/gpu/drm/tidss/tidss_encoder.c
> +++ b/drivers/gpu/drm/tidss/tidss_encoder.c
> @@ -55,12 +55,18 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
> return 0;
> }
>
> +static void tidss_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> + kfree(encoder);
> +}
> +
> static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> .atomic_check = tidss_encoder_atomic_check,
> };
>
> static const struct drm_encoder_funcs encoder_funcs = {
> - .destroy = drm_encoder_cleanup,
> + .destroy = tidss_encoder_destroy,
> };
>
> struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> @@ -69,7 +75,7 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
> struct drm_encoder *enc;
> int ret;
>
> - enc = devm_kzalloc(tidss->dev, sizeof(*enc), GFP_KERNEL);
> + enc = kzalloc(sizeof(*enc), GFP_KERNEL);
> if (!enc)
> return ERR_PTR(-ENOMEM);
>
> @@ -77,8 +83,10 @@ struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
>
> ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
> encoder_type, NULL);
> - if (ret < 0)
> + if (ret < 0) {
> + kfree(enc);
> return ERR_PTR(ret);
> + }
>
> drm_encoder_helper_add(enc, &encoder_helper_funcs);
>
> diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> index ff99b2dd4a17..798488948fc5 100644
> --- a/drivers/gpu/drm/tidss/tidss_plane.c
> +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> @@ -141,6 +141,14 @@ static void tidss_plane_atomic_disable(struct drm_plane *plane,
> dispc_plane_enable(tidss->dispc, tplane->hw_plane_id, false);
> }
>
> +static void drm_plane_destroy(struct drm_plane *plane)
> +{
> + struct tidss_plane *tplane = to_tidss_plane(plane);
> +
> + drm_plane_cleanup(plane);
> + kfree(tplane);
> +}
> +
> static const struct drm_plane_helper_funcs tidss_plane_helper_funcs = {
> .atomic_check = tidss_plane_atomic_check,
> .atomic_update = tidss_plane_atomic_update,
> @@ -151,7 +159,7 @@ static const struct drm_plane_funcs tidss_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> .reset = drm_atomic_helper_plane_reset,
> - .destroy = drm_plane_cleanup,
> + .destroy = drm_plane_destroy,
> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> };
> @@ -175,7 +183,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> BIT(DRM_MODE_BLEND_COVERAGE));
> int ret;
>
> - tplane = devm_kzalloc(tidss->dev, sizeof(*tplane), GFP_KERNEL);
> + tplane = kzalloc(sizeof(*tplane), GFP_KERNEL);
> if (!tplane)
> return ERR_PTR(-ENOMEM);
>
> @@ -190,7 +198,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> formats, num_formats,
> NULL, type, NULL);
> if (ret < 0)
> - return ERR_PTR(ret);
> + goto err;
>
> drm_plane_helper_add(&tplane->plane, &tidss_plane_helper_funcs);
>
> @@ -203,15 +211,19 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> default_encoding,
> default_range);
> if (ret)
> - return ERR_PTR(ret);
> + goto err;
>
> ret = drm_plane_create_alpha_property(&tplane->plane);
> if (ret)
> - return ERR_PTR(ret);
> + goto err;
>
> ret = drm_plane_create_blend_mode_property(&tplane->plane, blend_modes);
> if (ret)
> - return ERR_PTR(ret);
> + goto err;
>
> return tplane;
> +
> +err:
> + kfree(tplane);
> + return ERR_PTR(ret);
> }
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
