Re: [PATCH] drm/tidss: fix crash related to accessing freed memory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux