Re: [PATCH] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Chris Wilson <chris@chris-wilson.co.uk>]

On Tue, Apr 11, 2017 at 10:20:56PM +0100, Chris Wilson wrote:
> On Tue, Apr 11, 2017 at 11:01:57PM +0200, Takashi Iwai wrote:
> > On Tue, 11 Apr 2017 22:41:12 +0200,
> > Chris Wilson wrote:
> > Oh, this fell into a crack as it was sent just before my vacation.
> > 
> > About the change:
> > 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_lpe_audio.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > index 7a5b41b1c024..32000902a204 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > > @@ -131,8 +131,12 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
> > > >  
> > > >  static void lpe_audio_platdev_destroy(struct drm_i915_private *dev_priv)
> > > >  {
> > > > -	platform_device_unregister(dev_priv->lpe_audio.platdev);
> > > > -	kfree(dev_priv->lpe_audio.platdev->dev.dma_mask);
> > > > +	struct platform_device *platdev = dev_priv->lpe_audio.platdev;
> > > > +
> > > > +	kfree(platdev->dev.dma_mask);
> > > > +	platdev->dev.dma_mask = NULL;
> > > > +
> > > > +	platform_device_unregister(platdev);
> > 
> > I'm not sure whether it's good idea to fiddle dma_mask bits before the
> > unregister call.  Interestingly, this is the only driver that calls
> > kfree() for pdev's dma_mask.  Either we do something wrong, or
> > everyone forgot this?
> 
> Afaict, everyone else chose the blissful ignorance strategy and we are
> the only fools to try and free the dma_mask.
> 
> Would you feel more comfortable with:
> 
> 	void *mask = platdev->dev.dma_mask;
> 
> 	platform_device_unregister(platdev);
> 
> 	/* XXX see platform_device_register_full():
> 	 * "This memory isn't freed when the device is put."
> 	 * It's not clear why it hasn't been fixed in a decade...
> 	 */
> 	kfree(mask);

Still has the issue that unregister may not the final put, it should but
still...

I think we just leak the memory. We are not the owner and so shouldn't
be fiddling around trying to free it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx