Re: [PATCH] drm/bridge/synopsys: dw-hdmi: Fix memleak in __dw_hdmi_remove

[Russell King - ARM Linux]

On Mon, Mar 05, 2018 at 03:45:55PM +0800, Jeffy Chen wrote:
> The platform_device_register_full() will allocate dma_mask for
> hdmi->audio, so we should free before platform_device_unregister().
> 
> Reported by kmemleak:
> unreferenced object 0xffffffc0ef70ff00 (size 128):
>   comm "kworker/4:1", pid 123, jiffies 4294670080 (age 189.604s)
>   hex dump (first 32 bytes):
>     ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<0000000021946f44>] kmemleak_alloc+0x58/0x8c
>     [<000000009c43890d>] kmem_cache_alloc_memcg_trace+0x18c/0x25c
>     [<000000000e17cd06>] platform_device_register_full+0x64/0x108
>     [<00000000418a0882>] __dw_hdmi_probe+0xb9c/0xcc0
>     [<00000000e0b720fd>] dw_hdmi_bind+0x30/0x88
>     [<000000009af347f6>] dw_hdmi_rockchip_bind+0x260/0x2e8
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> ---
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index f9802399cc0d..d9afdc59d4f4 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2567,8 +2567,10 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  
>  static void __dw_hdmi_remove(struct dw_hdmi *hdmi)
>  {
> -	if (hdmi->audio && !IS_ERR(hdmi->audio))
> +	if (hdmi->audio && !IS_ERR(hdmi->audio)) {
> +		kfree(hdmi->audio->dev.dma_mask);
>  		platform_device_unregister(hdmi->audio);
> +	}
>  	if (!IS_ERR(hdmi->cec))
>  		platform_device_unregister(hdmi->cec);

NAK.  This is a hack, plain and simple.

The audio device is created by platform_device_register_full(), and
the lifetime of that is managed by the driver model.  When it's time
to clean that up, it's handled by the platform support code in
drivers/base/platform.c.  It is not the driver's responsibility to
clean this up.

There's two issues here:
- Since you're kfree()ing it _before_ unregistering the device, the
  memory you're freeing may still be accessed by the DMA API on
  behalf of the device driver, so you're introducing a use-after-free
  bug.
- kfree()ing it _after_ unregistering the device means that you're then
  potentially accessing memory that has been kfree()'d (the underlying
  struct device) unless you save a pointer to it before unregistering
  and kfree() it after.

However, see the comment in platform_device_register_full() - if this
gets fixed at the core platform level (since every driver using
platform_device_register_full() with a DMA mask will suffer this
same problem, that's the place to fix it) adding a workaround at driver
level will introduce a non-obvious double-free bug.

I suspect the best solution to this is to arrange for struct
platform_device to also contain a DMA mask which
platform_device_register_full() can use, which means the DMA mask will
be freed automatically along with the rest of the platform device,
rather than separately allocating this.  There may be some resistance
to adding 64 bits to platform devices, but if we have lots of these
separately kmalloc'd DMA masks, we're actually wasting a lot more memory
not doing this (since kmalloc has a minimum size of the L1 cacheline
size.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up