All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/tidss: fix crash related to accessing freed memory
@ 2020-04-15  9:20 Tomi Valkeinen
  2020-04-15  9:29 ` Tomi Valkeinen
  2020-04-15 12:45 ` Laurent Pinchart
  0 siblings, 2 replies; 7+ messages in thread
From: Tomi Valkeinen @ 2020-04-15  9:20 UTC (permalink / raw)
  To: dri-devel, Laurent Pinchart, Daniel Vetter; +Cc: Tomi Valkeinen

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@ti.com>
---
 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);
+}
+
 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);
 }
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15  9:20 [PATCH] drm/tidss: fix crash related to accessing freed memory Tomi Valkeinen
@ 2020-04-15  9:29 ` Tomi Valkeinen
  2020-04-15 12:45 ` Laurent Pinchart
  1 sibling, 0 replies; 7+ messages in thread
From: Tomi Valkeinen @ 2020-04-15  9:29 UTC (permalink / raw)
  To: dri-devel, Laurent Pinchart, Daniel Vetter, Jyri Sarha

(Adding Jyri, forgot to add him)

On 15/04/2020 12:20, 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@ti.com>
> ---
>   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);
> +}
> +
>   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);
>   }
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15  9:20 [PATCH] drm/tidss: fix crash related to accessing freed memory Tomi Valkeinen
  2020-04-15  9:29 ` Tomi Valkeinen
@ 2020-04-15 12:45 ` Laurent Pinchart
  2020-04-15 12:52   ` Daniel Vetter
  2020-04-15 13:31   ` Tomi Valkeinen
  1 sibling, 2 replies; 7+ messages in thread
From: Laurent Pinchart @ 2020-04-15 12:45 UTC (permalink / raw)
  To: Tomi Valkeinen; +Cc: Daniel Vetter, dri-devel

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@ti.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15 12:45 ` Laurent Pinchart
@ 2020-04-15 12:52   ` Daniel Vetter
  2020-04-15 13:03     ` Laurent Pinchart
  2020-04-15 13:31   ` Tomi Valkeinen
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2020-04-15 12:52 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel

On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote:
> 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@ti.com>
> > ---
> >  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.

I'm working (well plan to at least) on some nice infrastructure so that
all this can be garbage collected again. I think embeddeding into the
top-level structure is only neat if you have a very simple device (and
then maybe just embed the drm_simple_kms thing). tidss didn't look quite
that simple, but maybe I'm missing the big picture ...

Oh and on the patch:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-Daniel

> 
> > +}
> > +
> >  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15 12:52   ` Daniel Vetter
@ 2020-04-15 13:03     ` Laurent Pinchart
  2020-04-15 17:49       ` Daniel Vetter
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2020-04-15 13:03 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel

Hi Daniel,

On Wed, Apr 15, 2020 at 02:52:43PM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote:
> > 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@ti.com>
> >> ---
> >>  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.
> 
> I'm working (well plan to at least) on some nice infrastructure so that
> all this can be garbage collected again. I think embeddeding into the
> top-level structure is only neat if you have a very simple device (and
> then maybe just embed the drm_simple_kms thing). tidss didn't look quite
> that simple, but maybe I'm missing the big picture ...

I think embedding is the best option when you have a fixed number of
CRTCs, encoders and planes. If they're variable but reasonably bounded,
embedding will waste a bit of memory, but massively simplify the code.
Even with the helpers you're working on, it will save memory as devres
allocates memory to track objects, and will certainly save CPU time too,
so it could be a net gain in the end. That's the method I recommend if
it can be used, but it may be a matter of taste.

> Oh and on the patch:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >> +}
> >> +
> >>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15 12:45 ` Laurent Pinchart
  2020-04-15 12:52   ` Daniel Vetter
@ 2020-04-15 13:31   ` Tomi Valkeinen
  1 sibling, 0 replies; 7+ messages in thread
From: Tomi Valkeinen @ 2020-04-15 13:31 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Daniel Vetter, dri-devel

Hi,

On 15/04/2020 15:45, Laurent Pinchart wrote:

>> +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.

This seemed the easiest way to fix this for 5.7-rcs, without doing too many changes all around that 
might cause conflicts. The allocs and frees are close to each other, in the same files, although 
there's repetition of course.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/tidss: fix crash related to accessing freed memory
  2020-04-15 13:03     ` Laurent Pinchart
@ 2020-04-15 17:49       ` Daniel Vetter
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2020-04-15 17:49 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Daniel Vetter, Tomi Valkeinen, dri-devel

On Wed, Apr 15, 2020 at 04:03:44PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Wed, Apr 15, 2020 at 02:52:43PM +0200, Daniel Vetter wrote:
> > On Wed, Apr 15, 2020 at 03:45:50PM +0300, Laurent Pinchart wrote:
> > > 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@ti.com>
> > >> ---
> > >>  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.
> > 
> > I'm working (well plan to at least) on some nice infrastructure so that
> > all this can be garbage collected again. I think embeddeding into the
> > top-level structure is only neat if you have a very simple device (and
> > then maybe just embed the drm_simple_kms thing). tidss didn't look quite
> > that simple, but maybe I'm missing the big picture ...
> 
> I think embedding is the best option when you have a fixed number of
> CRTCs, encoders and planes. If they're variable but reasonably bounded,
> embedding will waste a bit of memory, but massively simplify the code.
> Even with the helpers you're working on, it will save memory as devres
> allocates memory to track objects, and will certainly save CPU time too,
> so it could be a net gain in the end. That's the method I recommend if
> it can be used, but it may be a matter of taste.

For small drivers it doesn't really matter, but my experience with big
drivers is that if you smash everything into one place, you end up with
spaghetti code. In i915 we're working really hard to pull stuff out of the
massive i915 driver structure, simply to force better organization of the
code.

Separate allocations force a bit more thought about encapsulating objects
and putting code to the right place at least. That's kinda why I prefer
it.

At the end it's a tradeoff, so *shrug*
-Daniel

> 
> > Oh and on the patch:
> > 
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > >> +}
> > >> +
> > >>  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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-15 17:49 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15  9:20 [PATCH] drm/tidss: fix crash related to accessing freed memory Tomi Valkeinen
2020-04-15  9:29 ` Tomi Valkeinen
2020-04-15 12:45 ` Laurent Pinchart
2020-04-15 12:52   ` Daniel Vetter
2020-04-15 13:03     ` Laurent Pinchart
2020-04-15 17:49       ` Daniel Vetter
2020-04-15 13:31   ` Tomi Valkeinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.