Re: [PATCH v3] drm/i915: Fix use after free in lpe_audio_platdev_destroy()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
 
 	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);
-		kfree(pdev->dev.dma_mask);
 
 err_alloc:
 		platform_device_put(pdev);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux