Re: [PATCH] drm/cma-helper: Fix crash in fbdev error path

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Sep 29, 2018 at 07:12:00PM +0200, Noralf Trønnes wrote:
> Sergey Suloev reported a crash happening in drm_client_dev_hotplug()
> when fbdev had failed to register.
> 
> [    9.124598] vc4_hdmi 3f902000.hdmi: ASoC: Failed to create component debugfs directory
> [    9.147667] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok
> [    9.155184] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> [    9.166544] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> [    9.173840] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> [    9.181029] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
> [    9.188519] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> [    9.195690] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> [    9.203523] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> [    9.215032] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> [    9.274785] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> [    9.290246] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> [    9.297464] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [    9.304600] [drm] Driver supports precise vblank timestamp query.
> [    9.382856] vc4-drm soc:gpu: [drm:drm_fb_helper_fbdev_setup [drm_kms_helper]] *ERROR* Failed to set fbdev configuration
> [   10.404937] Unable to handle kernel paging request at virtual address 00330a656369768a
> [   10.441620] [00330a656369768a] address between user and kernel address ranges
> [   10.449087] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [   10.454762] Modules linked in: brcmfmac vc4 drm_kms_helper cfg80211 drm rfkill smsc95xx brcmutil usbnet drm_panel_orientation_quirks raspberrypi_hwmon bcm2835_dma crc32_ce pwm_bcm2835 bcm2835_rng virt_dma rng_core i2c_bcm2835 ip_tables x_tables ipv6
> [   10.477296] CPU: 2 PID: 45 Comm: kworker/2:1 Not tainted 4.19.0-rc5 #3
> [   10.483934] Hardware name: Raspberry Pi 3 Model B Rev 1.2 (DT)
> [   10.489966] Workqueue: events output_poll_execute [drm_kms_helper]
> [   10.596515] Process kworker/2:1 (pid: 45, stack limit = 0x000000007e8924dc)
> [   10.603590] Call trace:
> [   10.606259]  drm_client_dev_hotplug+0x5c/0xb0 [drm]
> [   10.611303]  drm_kms_helper_hotplug_event+0x30/0x40 [drm_kms_helper]
> [   10.617849]  output_poll_execute+0xc4/0x1e0 [drm_kms_helper]
> [   10.623616]  process_one_work+0x1c8/0x318
> [   10.627695]  worker_thread+0x48/0x428
> [   10.631420]  kthread+0xf8/0x128
> [   10.634615]  ret_from_fork+0x10/0x18
> [   10.638255] Code: 54000220 f9401261 aa1303e0 b4000141 (f9400c21)
> [   10.644456] ---[ end trace c75b4a4b0e141908 ]---
> 
> The reason for this is that drm_fbdev_cma_init() removes the drm_client
> when fbdev registration fails, but it doesn't remove the client from the
> drm_device client list. So the client list now has a pointer that points
> into the unknown and we have a 'use after free' situation.
> 
> It was decided during the drm_client API discussions that a client should
> not be allowed to remove itself. This was based on the experience with
> the fbdev/fbcon locking horror show.
> 
> So the solution is to just leave the drm_client hanging around doing
> nothing (it doesn't have any callbacks), and let it be removed when
> drm_dev_unregister() is called.

Slightly cleaner solution: Split drm_client_new into drm_client_alloc()
and drm_client_add(). Call the later only when initialization succeeds,
and only _add() will add it to the device list.

But this here gets the job done for -fixes, so:

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

But feel free to respin with the _alloc()/_add() idea.

> 
> Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
> Reported-by: Sergey Suloev <ssuloev@orpaltech.com>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> 
> This bug is in 4.19 so it should go into drm-misc-fixes right?
> (I have never applied a fix before)

Yup.

If you decide to push this, and do the _alloc/add() refactor afterwards
you need to request a backmerge from drm-misc maintainers (since the
refactor should go into drm-misc ofc).

Cheers, Daniel

> 
> Noralf.
> 
>  drivers/gpu/drm/drm_fb_cma_helper.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 47e0e2f6642d..d82bb4776e3c 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -168,22 +168,21 @@ struct drm_fbdev_cma *drm_fbdev_cma_init(struct drm_device *dev,
>  	fb_helper = &fbdev_cma->fb_helper;
>  
>  	ret = drm_client_new(dev, &fb_helper->client, "fbdev", NULL);
> -	if (ret)
> -		goto err_free;
> +	if (ret) {
> +		kfree(fbdev_cma);
> +		return ERR_PTR(ret);
> +	}
>  
>  	ret = drm_fb_helper_fbdev_setup(dev, fb_helper, &drm_fb_cma_helper_funcs,
>  					preferred_bpp, max_conn_count);
>  	if (ret)
> -		goto err_client_put;
> +		/*
> +		 * A drm_client is not allowed to remove itself, so it has to
> +		 * stay around until drm_dev_unregister().
> +		 */
> +		return ERR_PTR(ret);
>  
>  	return fbdev_cma;
> -
> -err_client_put:
> -	drm_client_release(&fb_helper->client);
> -err_free:
> -	kfree(fbdev_cma);
> -
> -	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(drm_fbdev_cma_init);
>  
> -- 
> 2.15.1
> 

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