On Thu, 03 Aug 2023 15:06:19 +0200, Takashi Iwai wrote: > > On Wed, 02 Aug 2023 19:06:05 +0200, > Curtis Malainey wrote: > > > > On Tue, Aug 1, 2023 at 11:42 PM Takashi Iwai <tiwai@xxxxxxx> wrote: > > > > > > On Tue, 01 Aug 2023 19:18:41 +0200, > > > cujomalainey@xxxxxxxxxxxx wrote: > > > > > > > > From: Curtis Malainey <cujomalainey@xxxxxxxxxxxx> > > > > > > > > The current implementation of how devices are released is valid for > > > > production use cases (root control of memory is handled by card_dev, all > > > > other devices are no-ops). > > > > > > > > This model does not work though in a kernel hacking environment where > > > > KASAN and delayed release on kobj is enabled. If the card_dev device is > > > > released before any of the child objects a use-after-free bug is caught > > > > by KASAN as the delayed release still has a reference to the devices > > > > that were freed by the card_dev release. Also both snd_card and snd_pcm > > > > both own two devices internally, so even if they released independently, > > > > the shared struct would result in another use after free. > > > > > > > > Solution is to move the child devices into their own memory so they can > > > > be handled independently and released on their own schedule. > > > > > > > > Signed-off-by: Curtis Malainey <cujomalainey@xxxxxxxxxxxx> > > > > Cc: Doug Anderson <dianders@xxxxxxxxxxxx> > > > > > > Thanks, it's an interesting bug. > > > > > > I'm not much against the proposed solution, but still this has to be > > > carefully evaluated. So, could you give more details about the bug > > > itself? It's related with CONFIG_DEBUG_KOBJECT_RELEASE, right? > > > In which condition it's triggered and how the UAF looks like? > > > > > > Hi Takashi > > > > Here is one of the stack traces we are seeing (for snd_pcm) > > > > [ 1098.876460] ================================================================== > > [ 1098.884763] BUG: KASAN: use-after-free in enqueue_timer+0xa0/0x628 > > [ 1098.884849] Write of size 8 at addr ffffff80cbdc6800 by task kworker/7:4/255 > > [ 1098.884888] > > [ 1098.884909] CPU: 7 PID: 255 Comm: kworker/7:4 Not tainted > > 5.15.117-lockdep-19614-g05bc12fd18a9 #1 > > 5a757fac993273e9fde5e8de9925ee0cebe8540f > > [ 1098.884961] Hardware name: Google Pompom (rev3+) with LTE (DT) > > [ 1098.884990] Workqueue: events kobject_delayed_cleanup > > [ 1098.885038] Call trace: > > [ 1098.885059] dump_backtrace+0x0/0x4e8 > > [ 1098.885095] show_stack+0x34/0x44 > > [ 1098.885129] dump_stack_lvl+0xdc/0x11c > > [ 1098.885165] print_address_description+0x30/0x2d8 > > [ 1098.885203] kasan_report+0x178/0x1e4 > > [ 1098.885237] __asan_report_store8_noabort+0x44/0x50 > > [ 1098.885276] enqueue_timer+0xa0/0x628 > > [ 1098.885309] internal_add_timer+0xa0/0x254 > > [ 1098.885346] __mod_timer+0x588/0xc4c > > [ 1098.885378] add_timer+0x74/0x94 > > [ 1098.885411] __queue_delayed_work+0x170/0x208 > > [ 1098.885447] queue_delayed_work_on+0x128/0x160 > > [ 1098.885483] kobject_put+0x278/0x2cc > > [ 1098.885517] put_device+0x30/0x48 > > [ 1098.885553] snd_ctl_dev_free+0xc8/0xe4 > > [ 1098.885590] __snd_device_free+0x124/0x1b8 > > [ 1098.885626] snd_device_free_all+0x148/0x184 > > [ 1098.885663] release_card_device+0x5c/0x180 > > [ 1098.885698] device_release+0xd4/0x1b4 > > [ 1098.885732] kobject_delayed_cleanup+0x130/0x304 > > [ 1098.885765] process_one_work+0x620/0x137c > > [ 1098.885802] worker_thread+0x43c/0xa08 > > [ 1098.885837] kthread+0x2e4/0x3a0 > > [ 1098.885872] ret_from_fork+0x10/0x20 > > [ 1098.885907] > > [ 1098.885926] Allocated by task 11891: > > [ 1098.885953] kasan_save_stack+0x38/0x68 > > [ 1098.885992] __kasan_kmalloc+0x90/0xac > > [ 1098.886026] kmem_cache_alloc_trace+0x258/0x40c > > [ 1098.886063] _snd_pcm_new+0xc4/0x2c8 > > [ 1098.886098] snd_pcm_new+0x5c/0x74 > > [ 1098.886132] loopback_pcm_new+0xa0/0x20c [snd_aloop] > > [ 1098.886194] loopback_probe+0x210/0x850 [snd_aloop] > > [ 1098.886235] platform_probe+0x150/0x1c8 > > [ 1098.886273] really_probe+0x274/0xa20 > > [ 1098.886308] __driver_probe_device+0x1b4/0x3b4 > > [ 1098.886344] driver_probe_device+0x78/0x1c0 > > [ 1098.886379] __device_attach_driver+0x1ac/0x2c8 > > [ 1098.886414] bus_for_each_drv+0x11c/0x190 > > [ 1098.886448] __device_attach+0x278/0x3c8 > > [ 1098.886482] device_initial_probe+0x2c/0x3c > > [ 1098.886517] bus_probe_device+0xbc/0x1c8 > > [ 1098.886550] device_add+0x1174/0x1630 > > [ 1098.886581] platform_device_add+0x3f8/0x6f8 > > [ 1098.886617] platform_device_register_full+0x36c/0x3f8 > > [ 1098.886656] 0xffffffc0032e3174 > > [ 1098.886691] do_one_initcall+0x1e8/0x91c > > [ 1098.886727] do_init_module+0x16c/0x444 > > [ 1098.886762] load_module+0x63e0/0x7f8c > > [ 1098.886795] __arm64_sys_finit_module+0x1e4/0x214 > > [ 1098.886830] invoke_syscall+0x98/0x278 > > [ 1098.886862] el0_svc_common+0x214/0x274 > > [ 1098.886894] do_el0_svc+0x9c/0x19c > > [ 1098.886926] el0_svc+0x5c/0xc0 > > [ 1098.886959] el0t_64_sync_handler+0x78/0x108 > > [ 1098.886994] el0t_64_sync+0x1a4/0x1a8 > > [ 1098.887027] > > [ 1098.887046] Freed by task 255: > > [ 1098.887071] kasan_save_stack+0x38/0x68 > > [ 1098.887106] kasan_set_track+0x28/0x3c > > [ 1098.887139] kasan_set_free_info+0x28/0x4c > > [ 1098.887174] ____kasan_slab_free+0x110/0x164 > > [ 1098.887209] __kasan_slab_free+0x18/0x28 > > [ 1098.887242] kfree+0x200/0x868 > > [ 1098.887275] snd_pcm_free+0x88/0x98 > > [ 1098.887311] snd_pcm_dev_free+0x48/0x5c > > [ 1098.887347] __snd_device_free+0x124/0x1b8 > > [ 1098.887384] snd_device_free_all+0xc8/0x184 > > [ 1098.887419] release_card_device+0x5c/0x180 > > [ 1098.887453] device_release+0xd4/0x1b4 > > [ 1098.887486] kobject_delayed_cleanup+0x130/0x304 > > [ 1098.887519] process_one_work+0x620/0x137c > > [ 1098.887555] worker_thread+0x43c/0xa08 > > [ 1098.887590] kthread+0x2e4/0x3a0 > > [ 1098.887623] ret_from_fork+0x10/0x20 > > > > A similar one is generated for snd_card if card_dev.release runs > > before ctl_dev.release. This patch is solving the same bug in both > > places at once. > > Hmm. It's still puzzling, and I'm not 100% sure whether your analysis > is right. The above shows it's freed by task 255, while the task > hitting UAF itself is 255. It might be rather the combination of > devres and delayed releases. > > With CONFIG_DEBUG_KOBJECT_RELEASE, the kernel should show the info of > each delayed release kobject. Could you give them together with the > Oops, so that we can see the flow how it hits UAF? Now I slowly understand what's happening. Essentially, it's because the *embedded* struct device is released asynchronously from the ALSA's resource management (via dev_free ops). The objects may be already released via card's device release (that calls snd_device_free_all()), while the release of each struct device that was already triggered beforehand can be delayed due to the debug option. You code change looks mostly fine, but as far as I see, compress_offload also needs a similar change. Meanwhile, rawmidi and hwdep do release the object via device release properly, so those are fine. Ditto for sequencer. And timer is only a global device, hence it must be fine. And, I believe we need a bit more detailed patch description. So, could you improve the patch description, split the change to each component (control, pcm and compress_offload), and resubmit? Also, maybe it's worth to change snd_device_initialize() to take the release (optional) argument, too, instead of setting it explicitly afterwards at each place. Already majority of callers will set own release callbacks. This can be done at one more additional patch at last. thanks, Takashi
