Re: [PATCH] ALSA: usb-audio: fix potential use after free issue when remove module snd-usb-audio

[Xu Yang <xu.yang_2@xxxxxxx>]

On Mon, May 20, 2024 at 12:29:15PM +0200, Takashi Iwai wrote:
> On Mon, 20 May 2024 19:03:49 +0200,
> Xu Yang wrote:
> > 
> > When remove module snd-usb-audio, snd_card_free_when_closed() will not
> > release the card resource if the card_dev refcount > 0 and

[...]

> > Then, even the userspace trying to cleanup the resources, kernel will not
> > touch the released code memory.
> 
> Hm, it's an interesting report.  Could you verify whether it's really
> hitting a module unload race?  The module refcount should have been
> non-zero when the device is still in use, and it should have prevented
> the module unloading.

Yes, the race does exist. I enable trace and got below output:
It seems that snd_usb_audio module refcnt is 0 after insmod completed. So
it can continue to be removed even it's still in use.

# echo 1 > /sys/kernel/tracing/events/module/enable
# cat /sys/kernel/tracing/trace_pipe &
[1] 522
# insmod snd-hwdep.ko
           <...>-527     [000] .....    42.147850: module_load: snd_hwdep
          insmod-527     [000] .....    42.148106: module_put: snd_hwdep call_site=do_init_module refcnt=1
# insmod snd-usbmidi-lib.ko
           <...>-529     [000] .....    46.723871: module_load: snd_usbmidi_lib
          insmod-529     [000] .....    46.724105: module_put: snd_usbmidi_lib call_site=do_init_module refcnt=1
# insmod snd-usb-audio.ko; sleep 2; rmmod snd-usb-audio.ko
           <...>-531     [000] .....    59.720852: module_get: snd_usbmidi_lib call_site=resolve_symbol refcnt=2
           <...>-531     [000] .....    59.720922: module_get: snd_hwdep call_site=resolve_symbol refcnt=2
           <...>-531     [000] .....    59.722334: module_load: snd_usb_audio
          insmod-531     [000] .....    59.893096: module_put: snd_usb_audio call_site=do_init_module refcnt=1
           rmmod-541     [000] .....    61.911391: module_free: snd_usb_audio
           rmmod-541     [000] .....    61.912291: module_put: snd_hwdep call_site=module_unload_free refcnt=1
           rmmod-541     [000] .....    61.912315: module_put: snd_usbmidi_lib call_site=module_unload_free refcnt=1

[   61.927058] Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP
[   61.933320] Modules linked in: snd_usbmidi_lib snd_hwdep [last unloaded: snd_usb_audio]
[   61.941320] CPU: 1 PID: 476 Comm: wireplumber Not tainted 6.6.23-06215-gc5317d88b3ec #719
[   61.949480] Hardware name: NXP i.MX93 11X11 EVK board (DT)
[   61.954950] pstate: 20400009 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   61.961900] pc : 0xffff80007a85fb88
[   61.965384] lr : snd_pcm_dev_free+0x28/0x5c
[   61.969567] sp : ffff800083f9bbf0
[   61.972869] x29: ffff800083f9bbf0 x28: ffff00000a0b8000 x27: 0000000000000000
[   61.979993] x26: dead000000000122 x25: dead000000000100 x24: ffff00000a2dea70
[   61.987117] x23: ffff800082819ce8 x22: 0000000000000000 x21: ffff00000a2de800
[   61.994241] x20: ffff00000a2de9a0 x19: ffff00000751ba00 x18: 0000000000000000
[   62.001368] x17: 0000000000000000 x16: 0000000000000000 x15: 0111289e3bb3ce7e
[   62.008489] x14: 000044678c0b24b6 x13: 00000000000000ab x12: ffff000007b23200
[   62.015613] x11: 0000000000000000 x10: 0000000000000a70 x9 : ffff800083f9b9e0
[   62.022737] x8 : ffff800083f9bba0 x7 : 0000000000000000 x6 : ffff00000a2ded98
[   62.029861] x5 : ffff8000811bedc4 x4 : dead000000000122 x3 : ffff00000b52dc80
[   62.036985] x2 : ffff000007f36240 x1 : ffff80007a85fb88 x0 : ffff00000751ba00
[   62.044112] Call trace:
[   62.046548]  0xffff80007a85fb88
[   62.049683]  __snd_device_free+0x50/0xd0
[   62.053602]  snd_device_free_all+0x4c/0x9c
[   62.057690]  release_card_device+0x24/0x90
[   62.061772]  device_release+0x34/0x90
[   62.065432]  kobject_put+0xa4/0x120
[   62.068913]  put_device+0x14/0x24
[   62.072224]  snd_card_file_remove+0xe4/0x178
[   62.076479]  snd_ctl_release+0xfc/0x114
[   62.080313]  snd_disconnect_release+0xcc/0x114
[   62.084747]  __fput+0xb4/0x274
[   62.087798]  __fput_sync+0x50/0x5c
[   62.091195]  __arm64_sys_close+0x38/0x7c
[   62.095113]  invoke_syscall+0x48/0x110
[   62.098857]  el0_svc_common.constprop.0+0xc0/0xe0
[   62.103554]  do_el0_svc+0x1c/0x28
[   62.106865]  el0_svc+0x40/0xe4
[   62.109918]  el0t_64_sync_handler+0x120/0x12c
[   62.114266]  el0t_64_sync+0x190/0x194
[   62.117933] Code: ???????? ???????? ???????? ???????? (????????)
[   62.124011] ---[ end trace 0000000000000000 ]---

Then I take some time to check why snd_usb_audio module refcnt is 0
even though the card_dev is in use. Finally I got below finding:

I build kernel and module with below configuration:

CONFIG_SOUND=y
CONFIG_SND=y
CONFIG_SND_USB=y
CONFIG_SND_USB_AUDIO=m

Then GCC will add -DMODULE when build snd-usb-audio as module, but will
not add -DMODULE when build sound/core/*.c.

When insmod snd-usb-audio.ko, it will create a snd card device and call:

snd_card_init()  // sound/core/init.c

  #ifdef MODULE
    WARN_ON(!module);
    card->module = module;
  #endif

However, MODULE is not defined for sound/core/init.c, then card->module
will keep NULL pointer. With this results, snd-usb-audio module refcnt
will not be a non-zero value.

> 
> Practically seen, replacing snd_card_free_when_closed() with
> snd_card_free() shouldn't be a big problem, and it'll work in most
> cases.  But there are always some corner cases that might lead to
> unexpected behavior.  So, let's try to analyze more exactly what's
> happening there at first.

With above finding, we needn't to replace snd_card_free_when_closed()
with snd_card_free(). We need to find a way to correctly handle module
refcnt since this should be a normal usecase.

Thanks,
Xu Yang

> 
> 
> thanks,
> 
> Takashi
>