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

Daniel Vetter daniel at ffwll.ch
Mon Nov 5 09:16:40 UTC 2018


On Sun, Nov 04, 2018 at 08:10:57PM +0300, Sergey Suloev wrote:
> To be exact, the issue does not happen in case I use default kernel config
> but only with the custom one.

Can you pls grab a new backtrace on latest kernels?

Thanks, Daniel

> 
> 
> On 10/2/18 2:14 PM, Noralf Trønnes wrote:
> > 
> > Den 01.10.2018 21.45, skrev Noralf Trønnes:
> > > 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.
> > > 
> > > Split drm_client_new() into drm_client_init() and drm_client_add()
> > > to fix
> > > removal in the error path.
> > > 
> > > Fixes: 894a677f4b3e ("drm/cma-helper: Use the generic fbdev emulation")
> > > Reported-by: Sergey Suloev <ssuloev at orpaltech.com>
> > > Cc: Stefan Wahren <stefan.wahren at i2se.com>
> > > Cc: Eric Anholt <eric at anholt.net>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > ---
> > 
> > Applied to drm-misc-fixes, thanks for the review.
> > 
> > Noralf.
> > 
> > > Changes since version 2:
> > > - Fix drm_client_new() ref in docs (Daniel)
> > > - Expand drm_client_add() docs (Daniel)
> > > 
> > > Changes since version 1:
> > > - Split drm_client_new() instead of letting drm_client hang around
> > > (Daniel)
> > > 
> > >   drivers/gpu/drm/drm_client.c        | 35
> > > ++++++++++++++++++++++++++---------
> > >   drivers/gpu/drm/drm_fb_cma_helper.c |  4 +++-
> > >   drivers/gpu/drm/drm_fb_helper.c     |  4 +++-
> > >   include/drm/drm_client.h            |  5 +++--
> > >   4 files changed, 35 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > index 17d9a64e885e..fc03d26fcacc 100644
> > > --- a/drivers/gpu/drm/drm_client.c
> > > +++ b/drivers/gpu/drm/drm_client.c
> > > @@ -63,20 +63,21 @@ static void drm_client_close(struct
> > > drm_client_dev *client)
> > >   EXPORT_SYMBOL(drm_client_close);
> > >     /**
> > > - * drm_client_new - Create a DRM client
> > > + * drm_client_init - Initialise a DRM client
> > >    * @dev: DRM device
> > >    * @client: DRM client
> > >    * @name: Client name
> > >    * @funcs: DRM client functions (optional)
> > >    *
> > > + * This initialises the client and opens a &drm_file. Use
> > > drm_client_add() to complete the process.
> > >    * The caller needs to hold a reference on @dev before calling
> > > this function.
> > >    * The client is freed when the &drm_device is unregistered. See
> > > drm_client_release().
> > >    *
> > >    * Returns:
> > >    * Zero on success or negative error code on failure.
> > >    */
> > > -int drm_client_new(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > -           const char *name, const struct drm_client_funcs *funcs)
> > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > +            const char *name, const struct drm_client_funcs *funcs)
> > >   {
> > >       int ret;
> > >   @@ -95,10 +96,6 @@ int drm_client_new(struct drm_device *dev,
> > > struct drm_client_dev *client,
> > >       if (ret)
> > >           goto err_put_module;
> > >   -    mutex_lock(&dev->clientlist_mutex);
> > > -    list_add(&client->list, &dev->clientlist);
> > > -    mutex_unlock(&dev->clientlist_mutex);
> > > -
> > >       drm_dev_get(dev);
> > >         return 0;
> > > @@ -109,13 +106,33 @@ int drm_client_new(struct drm_device *dev,
> > > struct drm_client_dev *client,
> > >         return ret;
> > >   }
> > > -EXPORT_SYMBOL(drm_client_new);
> > > +EXPORT_SYMBOL(drm_client_init);
> > > +
> > > +/**
> > > + * drm_client_add - Add client to the device list
> > > + * @client: DRM client
> > > + *
> > > + * Add the client to the &drm_device client list to activate its
> > > callbacks.
> > > + * @client must be initialized by a call to drm_client_init(). After
> > > + * drm_client_add() it is no longer permissible to call
> > > drm_client_release()
> > > + * directly (outside the unregister callback), instead cleanup will
> > > happen
> > > + * automatically on driver unload.
> > > + */
> > > +void drm_client_add(struct drm_client_dev *client)
> > > +{
> > > +    struct drm_device *dev = client->dev;
> > > +
> > > +    mutex_lock(&dev->clientlist_mutex);
> > > +    list_add(&client->list, &dev->clientlist);
> > > +    mutex_unlock(&dev->clientlist_mutex);
> > > +}
> > > +EXPORT_SYMBOL(drm_client_add);
> > >     /**
> > >    * drm_client_release - Release DRM client resources
> > >    * @client: DRM client
> > >    *
> > > - * Releases resources by closing the &drm_file that was opened by
> > > drm_client_new().
> > > + * Releases resources by closing the &drm_file that was opened by
> > > drm_client_init().
> > >    * It is called automatically if the &drm_client_funcs.unregister
> > > callback is _not_ set.
> > >    *
> > >    * This function should only be called from the unregister
> > > callback. An exception
> > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > index 47e0e2f6642d..fb0dfc62b1b6 100644
> > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > > @@ -167,7 +167,7 @@ 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);
> > > +    ret = drm_client_init(dev, &fb_helper->client, "fbdev", NULL);
> > >       if (ret)
> > >           goto err_free;
> > >   @@ -176,6 +176,8 @@ struct drm_fbdev_cma
> > > *drm_fbdev_cma_init(struct drm_device *dev,
> > >       if (ret)
> > >           goto err_client_put;
> > >   +    drm_client_add(&fb_helper->client);
> > > +
> > >       return fbdev_cma;
> > >     err_client_put:
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > > b/drivers/gpu/drm/drm_fb_helper.c
> > > index a504a5e05676..e9095537e0c0 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -3229,13 +3229,15 @@ int drm_fbdev_generic_setup(struct
> > > drm_device *dev, unsigned int preferred_bpp)
> > >       if (!fb_helper)
> > >           return -ENOMEM;
> > >   -    ret = drm_client_new(dev, &fb_helper->client, "fbdev",
> > > &drm_fbdev_client_funcs);
> > > +    ret = drm_client_init(dev, &fb_helper->client, "fbdev",
> > > &drm_fbdev_client_funcs);
> > >       if (ret) {
> > >           kfree(fb_helper);
> > >           DRM_DEV_ERROR(dev->dev, "Failed to register client: %d\n",
> > > ret);
> > >           return ret;
> > >       }
> > >   +    drm_client_add(&fb_helper->client);
> > > +
> > >       fb_helper->preferred_bpp = preferred_bpp;
> > >         ret = drm_fbdev_client_hotplug(&fb_helper->client);
> > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> > > index 989f8e52864d..971bb7853776 100644
> > > --- a/include/drm/drm_client.h
> > > +++ b/include/drm/drm_client.h
> > > @@ -87,9 +87,10 @@ struct drm_client_dev {
> > >       struct drm_file *file;
> > >   };
> > >   -int drm_client_new(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > -           const char *name, const struct drm_client_funcs *funcs);
> > > +int drm_client_init(struct drm_device *dev, struct drm_client_dev
> > > *client,
> > > +            const char *name, const struct drm_client_funcs *funcs);
> > >   void drm_client_release(struct drm_client_dev *client);
> > > +void drm_client_add(struct drm_client_dev *client);
> > >     void drm_client_dev_unregister(struct drm_device *dev);
> > >   void drm_client_dev_hotplug(struct drm_device *dev);
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list