[PATCH v2] drm/cma-helper: Fix crash in fbdev error path
Noralf Trønnes
noralf at tronnes.org
Mon Oct 1 19:32:37 UTC 2018
Den 01.10.2018 18.44, skrev Daniel Vetter:
> On Mon, Oct 01, 2018 at 04:57:32PM +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.
>>
>> 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>
>> ---
>>
>> Changes since version 1:
>> - Split drm_client_new() instead of letting drm_client hang around (Daniel)
>>
>>
>> Just to make sure I don't break anything, would this procedure do the right thing:
>>
>> dim update-branches
>> dim checkout drm-misc-fixes
> I recommend a
>
> dim create-worktree drm-misc-fixes
>
> here so that you have a separate checkout. Avoids accidentally thrashin
> stuff when switching between branches.
Nice, I didn't know about git worktree.
>> curl -sS https://patchwork.freedesktop.org/patch/$1/mbox/ > /tmp/patch
>> cat /tmp/patch | dim apply-branch drm-misc-fixes
>>
>> <build test>
>>
>> dim push-branch drm-misc-fixes
> Yup. On the patch, with the nits below addressed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
>>
>> drivers/gpu/drm/drm_client.c | 29 +++++++++++++++++++++--------
>> 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, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
>> index 17d9a64e885e..b466e08931fa 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,7 +106,23 @@ 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.
> I think this needs a few more details clarified in the doc:
>
> "@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, 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
> Grep says you missed one occurrence of drm_client_new() later on in one of
> the kerneldoc comments.
>
>> 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);
> Shouldn't we move this to later on, with an explicit drm_client_release()
> cleanup path, just for safety? Otoh, this is the minimal change, so maybe
> in a follow-up patch?
Yep, I'll do that later.
Thanks,
Noralf.
> Cheers, Daniel
>
>> +
>> 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);
>> --
>> 2.15.1
>>
More information about the dri-devel
mailing list