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

[Takashi Iwai <tiwai@xxxxxxx>]

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
> usb_audio_disconnect() will return directly, finally kernel will release
> the module resource. Then if the userspace continue to cleanup sound card
> resources, such as close controlC0, the closing path will touch this
> modules' code, unfortunately it just got released and the kernel will
> dump below error:
> 
> [  183.450073] Internal error: Oops: 0000000086000007 [#1] PREEMPT SMP
> [  183.456345] Modules linked in: snd_usbmidi_lib snd_hwdep [last unloaded: snd_usb_audio]
> [  183.464373] CPU: 0 PID: 537 Comm: wireplumber Not tainted 6.6.23-06215-gc5317d88b3ec #708
> [  183.472552] Hardware name: NXP i.MX93 11X11 EVK board (DT)
> [  183.478039] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  183.485004] pc : 0xffff80007c19a3b0
> [  183.488507] lr : snd_pcm_dev_free+0x3c/0x70
> [  183.492708] sp : ffff800085c77af0
> [  183.496030] x29: ffff800085c77af0 x28: dead000000000122 x27: ffff0000079f8090
> [  183.503188] x26: ffff0000079f8090 x25: ffff00000e78c270 x24: ffff00000e78c000
> [  183.510336] x23: ffff00000e78c1a8 x22: ffff00000e78c000 x21: ffff00000b6a2718
> [  183.517486] x20: ffff800082608180 x19: ffff00000d2d7400 x18: 0000000000000000
> [  183.524635] x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffa0000b90
> [  183.531786] x14: 0000000000000000 x13: 0000000000000000 x12: ffff700010b8ef49
> [  183.538939] x11: 1ffff00010b8ef48 x10: ffff700010b8ef48 x9 : 0000000000000000
> [  183.546086] x8 : ffff800085c77a50 x7 : ffff00006ccd77b0 x6 : 0000000000000003
> [  183.553236] x5 : 00000000000000c0 x4 : ffff00000ea57000 x3 : dfff800000000000
> [  183.560386] x2 : 0000000000000007 x1 : ffff80007c19a3b0 x0 : ffff00000d2d7400
> [  183.567539] Call trace:
> [  183.569992]  0xffff80007c19a3b0
> [  183.573134]  __snd_device_free+0x94/0x16c
> [  183.577156]  snd_device_free_all+0x70/0xe8
> [  183.581258]  release_card_device+0x30/0xc0
> [  183.585363]  device_release+0x50/0x10c
> [  183.589124]  kobject_put+0xe0/0x184
> [  183.592634]  put_device+0x14/0x24
> [  183.595954]  snd_card_file_remove+0x158/0x22c
> [  183.600322]  snd_ctl_release+0x174/0x194
> [  183.604256]  snd_disconnect_release+0x128/0x178
> [  183.608798]  __fput+0x160/0x3d0
> [  183.611955]  __fput_sync+0x74/0x84
> [  183.615370]  __arm64_sys_close+0x4c/0x8c
> [  183.619302]  invoke_syscall+0x60/0x184
> [  183.623072]  el0_svc_common.constprop.0+0x114/0x13c
> [  183.627960]  do_el0_svc+0x30/0x40
> [  183.631288]  el0_svc+0x38/0x70
> [  183.634356]  el0t_64_sync_handler+0x120/0x12c
> [  183.638724]  el0t_64_sync+0x190/0x194
> [  183.642403] Code: ???????? ???????? ???????? ???????? (????????)
> [  183.648497] ---[ end trace 0000000000000000 ]---
> 
> To fix the issue, use snd_card_free() to release all resources (including
> all files attached to the card_dev) instead snd_card_free_when_closed().
> 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.

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.


thanks,

Takashi

> 
> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> ---
>  sound/usb/card.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 1b2edc0fd2e9..5e799f147eb5 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -992,7 +992,7 @@ static void usb_audio_disconnect(struct usb_interface *intf)
>  	if (chip->num_interfaces <= 0) {
>  		usb_chip[chip->index] = NULL;
>  		mutex_unlock(&register_mutex);
> -		snd_card_free_when_closed(card);
> +		snd_card_free(card);
>  	} else {
>  		mutex_unlock(&register_mutex);
>  	}
> -- 
> 2.34.1
>