On Wed, Apr 12, 2017 at 01:41:05PM +0200, Takashi Iwai wrote:
> On Wed, 12 Apr 2017 10:52:54 +0200,
> Ville Syrjälä wrote:
> >
> > On Wed, Apr 12, 2017 at 09:31:39AM +0100, Chris Wilson wrote:
> > > [31908.547136] BUG: KASAN: use-after-free in intel_lpe_audio_teardown+0x78/0xb0 [i915] at addr ffff8801f7788358
> > > [31908.547297] Read of size 8 by task drv_selftest/3781
> > > [31908.547405] CPU: 0 PID: 3781 Comm: drv_selftest Tainted: G BU W 4.10.0+ #451
> > > [31908.547553] Hardware name: / , BIOS PYBSWCEL.86A.0027.2015.0507.1758 05/07/2015
> > > [31908.547682] Call Trace:
> > > [31908.547772] dump_stack+0x68/0x9f
> > > [31908.547857] kasan_object_err+0x1c/0x70
> > > [31908.547947] kasan_report_error+0x1f1/0x4f0
> > > [31908.548038] ? kfree+0xaa/0x170
> > > [31908.548121] kasan_report+0x34/0x40
> > > [31908.548211] ? klist_children_get+0x20/0x30
> > > [31908.548472] ? intel_lpe_audio_teardown+0x78/0xb0 [i915]
> > > [31908.548567] __asan_load8+0x5e/0x70
> > > [31908.548824] intel_lpe_audio_teardown+0x78/0xb0 [i915]
> > > [31908.549080] intel_audio_deinit+0x28/0x80 [i915]
> > > [31908.549315] i915_driver_unload+0xe4/0x360 [i915]
> > > [31908.549551] ? i915_driver_load+0x1d70/0x1d70 [i915]
> > > [31908.549651] ? trace_hardirqs_on+0xd/0x10
> > > [31908.549885] i915_pci_remove+0x23/0x30 [i915]
> > > [31908.549978] pci_device_remove+0x5c/0x100
> > > [31908.550069] device_release_driver_internal+0x1db/0x2e0
> > > [31908.550165] driver_detach+0x68/0xc0
> > > [31908.550256] bus_remove_driver+0x8b/0x150
> > > [31908.550346] driver_unregister+0x3e/0x60
> > > [31908.550439] pci_unregister_driver+0x1d/0x110
> > > [31908.550531] ? find_module_all+0x7a/0xa0
> > > [31908.550791] i915_exit+0x1a/0x87 [i915]
> > > [31908.550881] SyS_delete_module+0x264/0x2c0
> > > [31908.550971] ? free_module+0x430/0x430
> > > [31908.551064] ? trace_hardirqs_off_caller+0x16/0x110
> > > [31908.551159] ? trace_hardirqs_on_caller+0x16/0x280
> > > [31908.551256] ? trace_hardirqs_on_thunk+0x1a/0x1c
> > > [31908.551350] entry_SYSCALL_64_fastpath+0x1c/0xb1
> > > [31908.551440] RIP: 0033:0x7f1d67312ec7
> > > [31908.551520] RSP: 002b:00007ffebe34e888 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
> > > [31908.551650] RAX: ffffffffffffffda RBX: ffffffff811123f6 RCX: 00007f1d67312ec7
> > > [31908.551743] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000560d0af476b8
> > > [31908.551837] RBP: ffff880233d87f98 R08: 0000000000000000 R09: 00007ffebe34e8b8
> > > [31908.551930] R10: 00007f1d68adf8c0 R11: 0000000000000206 R12: 0000000000000000
> > > [31908.552023] R13: 0000560d0af46440 R14: 0000000000000034 R15: 00007ffebe34d860
> > > [31908.552121] ? trace_hardirqs_off_caller+0x16/0x110
> > > [31908.552217] Object at ffff8801f7788000, in cache kmalloc-2048 size: 2048
> > > [31908.552306] Allocated:
> > > [31908.552377] PID = 3781
> > > [31908.552456] save_stack_trace+0x16/0x20
> > > [31908.552539] kasan_kmalloc+0xee/0x190
> > > [31908.552627] __kmalloc+0xdb/0x1b0
> > > [31908.552713] platform_device_alloc+0x27/0x90
> > > [31908.552804] platform_device_register_full+0x36/0x220
> > > [31908.553066] intel_lpe_audio_init+0x41e/0x570 [i915]
> > > [31908.553320] intel_audio_init+0xd/0x40 [i915]
> > > [31908.553552] i915_driver_load+0x13f5/0x1d70 [i915]
> > > [31908.553788] i915_pci_probe+0x65/0xe0 [i915]
> > > [31908.553881] pci_device_probe+0xda/0x140
> > > [31908.553969] driver_probe_device+0x400/0x660
> > > [31908.554058] __driver_attach+0x11c/0x120
> > > [31908.554147] bus_for_each_dev+0xe6/0x150
> > > [31908.554237] driver_attach+0x26/0x30
> > > [31908.554325] bus_add_driver+0x26b/0x3b0
> > > [31908.554412] driver_register+0xce/0x190
> > > [31908.554502] __pci_register_driver+0xaf/0xc0
> > > [31908.554589] 0xffffffffa0550063
> > > [31908.554675] do_one_initcall+0x8b/0x1e0
> > > [31908.554764] do_init_module+0x102/0x325
> > > [31908.554852] load_module+0x3aad/0x45e0
> > > [31908.554944] SyS_finit_module+0x169/0x1a0
> > > [31908.555033] entry_SYSCALL_64_fastpath+0x1c/0xb1
> > > [31908.555119] Freed:
> > > [31908.555188] PID = 3781
> > > [31908.555266] save_stack_trace+0x16/0x20
> > > [31908.555349] kasan_slab_free+0xb0/0x180
> > > [31908.555436] kfree+0xaa/0x170
> > > [31908.555520] platform_device_release+0x76/0x80
> > > [31908.555610] device_release+0x45/0xe0
> > > [31908.555698] kobject_put+0x11f/0x260
> > > [31908.555785] put_device+0x12/0x20
> > > [31908.555871] platform_device_unregister+0x1b/0x20
> > > [31908.556135] intel_lpe_audio_teardown+0x5c/0xb0 [i915]
> > > [31908.556390] intel_audio_deinit+0x28/0x80 [i915]
> > > [31908.556622] i915_driver_unload+0xe4/0x360 [i915]
> > > [31908.556858] i915_pci_remove+0x23/0x30 [i915]
> > > [31908.556948] pci_device_remove+0x5c/0x100
> > > [31908.557037] device_release_driver_internal+0x1db/0x2e0
> > > [31908.557129] driver_detach+0x68/0xc0
> > > [31908.557217] bus_remove_driver+0x8b/0x150
> > > [31908.557304] driver_unregister+0x3e/0x60
> > > [31908.557394] pci_unregister_driver+0x1d/0x110
> > > [31908.557653] i915_exit+0x1a/0x87 [i915]
> > > [31908.557741] SyS_delete_module+0x264/0x2c0
> > > [31908.557834] entry_SYSCALL_64_fastpath+0x1c/0xb1
> > > [31908.557919] Memory state around the buggy address:
> > > [31908.558005] ffff8801f7788200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > > [31908.558127] ffff8801f7788280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > > [31908.558255] >ffff8801f7788300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > > [31908.558374] ^
> > > [31908.558467] ffff8801f7788380: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > > [31908.558595] ffff8801f7788400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> > >
> > > v2: Just leak the memory (8 bytes) as freeing it ourselves is not safe,
> > > and we need to coordinate a proper fix in platform_device itself.
> > > v3: Use dma_coerce_mask_and_coherent() to skip the kmalloc
> > >
> > > Fixes: eef57324d926 ("drm/i915: setup bridge for HDMI LPE audio driver")
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > > Cc: Jerome Anand <jerome.anand@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Cc: Takashi Iwai <tiwai@xxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_lpe_audio.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > index d8ca187ae001..dace2fb3154f 100644
> > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
> > > @@ -108,7 +108,6 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
> > > pinfo.num_res = 2;
> > > pinfo.data = pdata;
> > > pinfo.size_data = sizeof(*pdata);
> > > - pinfo.dma_mask = DMA_BIT_MASK(32);
> > >
> > > spin_lock_init(&pdata->lpe_audio_slock);
> > >
> > > @@ -119,6 +118,8 @@ lpe_audio_platdev_create(struct drm_i915_private *dev_priv)
> > > goto err;
> > > }
> > >
> > > + dma_coerce_mask_and_coherent(&platdev->dev, DMA_BIT_MASK(32));
> > > +
> >
> > Not sure how racy that is since we've already registered the platdev at
> > that point. The whole platform_register_full() API looks misdesigned to
> > me since you can't do stuff between alloc and register.
> >
> > We could shovel the dma_coerce_mask_and_coherent() call into
> > platform_register_full() itself I suppose. Or we just stop using the
> > register_full() stuff and do each step ourselves, but that looks a bit
> > tedious.
>
> Agreed. Through a quick glance, I couldn't find any caller that uses
> dev_mask and coherent_mask individually. We can clean up the whole
> mess like below.
>
> The platform_device_register_full() doesn't necessarily support all
> use-cases. If anyone really needs the separated dev_mask from the
> coherent_dma_mask, they should do manually.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index c4af00385502..d7e160b212b2 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -504,21 +504,8 @@ struct platform_device *platform_device_register_full(
> pdev->dev.parent = pdevinfo->parent;
> pdev->dev.fwnode = pdevinfo->fwnode;
>
> - if (pdevinfo->dma_mask) {
> - /*
> - * This memory isn't freed when the device is put,
> - * I don't have a nice idea for that though. Conceptually
> - * dma_mask in struct device should not be a pointer.
> - * See http://thread.gmane.org/gmane.linux.kernel.pci/9081
> - */
> - pdev->dev.dma_mask =
> - kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL);
> - if (!pdev->dev.dma_mask)
> - goto err;
> -
> - *pdev->dev.dma_mask = pdevinfo->dma_mask;
> - pdev->dev.coherent_dma_mask = pdevinfo->dma_mask;
> - }
> + if (pdevinfo->dma_mask)
> + dma_coerce_mask_and_coherent(&pdev->dev, pdevinfo->dma_mask);
A further question is whether this should actually check for the error
and propagate it to the caller. The old code silently assumed the DMA
mask was acceptable, but the new code will actually check for that.
>
> ret = platform_device_add_resources(pdev,
> pdevinfo->res, pdevinfo->num_res);
> @@ -541,7 +528,6 @@ struct platform_device *platform_device_register_full(
> if (ret) {
> err:
> ACPI_COMPANION_SET(&pdev->dev, NULL);
Unrelated, but I also have to wonder what this guys is doing here.
Some kind of misplaced thing, or a layering violation methinks.
The code never itself set this so I don't see why it would have
to clear it either.
> - kfree(pdev->dev.dma_mask);
>
> err_alloc:
> platform_device_put(pdev);
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
