Re: [PATCH] ALSA: hda: Serialize virtual-master hook registration

[Takashi Iwai <tiwai@suse.de>]

On Tue, 13 Oct 2026 09:54:55 +0200,
Cen Zhang wrote:
> 
> An HDA virtual-master hook must have its private argument initialized
> before a control callback can invoke it. snd_hda_add_vmaster_hook()
> installs the trampoline through snd_ctl_add_vmaster_hook(), which stores
> master->hook before master->hook_private_data without serialization.
> 
> When a codec with a vmaster mute hook binds to an already registered
> card, snd_hda_gen_build_controls() publishes and initializes the Master
> Playback Switch before installing the hook. A native write that changes
> the switch can then run in the following order:
> 
>   1. The codec bind path publishes and initializes the master control in
>      __snd_hda_add_vmaster(), then calls snd_hda_add_vmaster_hook().
>   2. The hook setter stores master->hook, but has not yet stored its
>      private argument.
>   3. SNDRV_CTL_IOCTL_ELEM_WRITE takes controls_rwsem and calls
>      master_put() while the setter remains between those stores.
>   4. master_put() sees the callback and invokes vmaster_hook() with a
>      NULL argument. The trampoline dereferences it before the setter
>      can complete publication.
> 
> The write lock held by snd_ctl_elem_write() does not exclude the hook
> setter, so this can cause a NULL pointer dereference. Take the same
> controls_rwsem for writing in snd_hda_add_vmaster_hook() so the callback
> and its argument are installed together with respect to native control
> callbacks. Release it when registration returns, before the caller
> synchronizes the hook.
> 
> KASAN report as below:
> 
>     Oops: general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] SMP KASAN NOPTI
>     KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
>     CPU: 1 UID: 0 PID: 499 Comm: alsa-vmaster-tr Not tainted 7.2.0-rc6-pmb-bt-functional-v1+ #1 PREEMPT(lazy)
>     Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
>     RIP: 0010:vmaster_hook+0x35/0xb0
>     Code: f5 53 48 89 fb 48 83 ec 08 e8 57 f0 9a fc eb 50 e8 50 f0 9a fc 48 8d 7b 08 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 75 59 48 8d 7b 10 48 8b 43 08 48 ba 00 00 00 00 00 fc
>     RSP: 0018:ffff888117647cd0 EFLAGS: 00010212
>     RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff84e9ff50
>     RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000008
>     RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000001
>     R10: ffffffff89427457 R11: ffff888115ad1d00 R12: ffffffff84e9ff30
>     R13: ffff888114943ca0 R14: 0000000000000000 R15: 0000000000000001
>     FS:  00007fe8167b3780(0000) GS:ffff88826b7e3000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 0000557c8c2842a8 CR3: 000000011579f005 CR4: 0000000000770ef0
>     PKRU: 55555554
>     Call Trace:
>      <TASK>
>      master_put+0x20e/0x2b0
>      snd_ctl_elem_write+0x37f/0x5e0
>      snd_ctl_ioctl+0x997/0xdd0
>      ? __pfx_snd_ctl_ioctl+0x10/0x10
>      ? srso_alias_return_thunk+0x5/0xfbef5
>      ? selinux_file_ioctl+0xaa/0x280
>      ? __pfx_snd_ctl_ioctl+0x10/0x10
>      __x64_sys_ioctl+0x163/0x1d0
>      do_syscall_64+0x115/0x6a0
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
>     RIP: 0033:0x7fe8168c291b
>     Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
>     RSP: 002b:00007fffab0cb150 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>     RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007fe8168c291b
>     RDX: 00007fffab0cb310 RSI: 00000000c4c85513 RDI: 0000000000000003
>     RBP: 00007fffab0cb310 R08: 0000000000000000 R09: 0000000000000000
>     R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
>     R13: 00007fffab0cb1c0 R14: 0000000000000001 R15: 0000557c4f71fd10
>      </TASK>
>     Modules linked in:
>     ---[ end trace 0000000000000000 ]---
> 
> Fixes: ee52e56e7b12 ("ALSA: hda - Fix mute-LED fixed mode")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> 
> diff --git a/sound/hda/common/codec.c b/sound/hda/common/codec.c
> index 18838a45e6f72b838eace2d7b21d7a0a3b21017b..029e56ae78727c8cb3c3edbace8b27a5615c2a3c 100644
> --- a/sound/hda/common/codec.c
> +++ b/sound/hda/common/codec.c
> @@ -2024,6 +2024,8 @@ int snd_hda_add_vmaster_hook(struct hda_codec *codec,
>  {
>  	if (!hook->hook || !hook->sw_kctl)
>  		return 0;
> +	/* Serialize hook registration with control callbacks. */
> +	guard(rwsem_write)(&codec->card->controls_rwsem);
>  	hook->codec = codec;
>  	snd_ctl_add_vmaster_hook(hook->sw_kctl, vmaster_hook, hook);
>  	return 0;

It's not nice to use such a low-level guard here in the upper driver.
So I guess it's better to move the rwsem guard into
snd_ctl_add_vmaster_hook().  But it'd need to pass the card pointer,
too.

Alternatively, you can rearrange the assignment in
snd_ctl_add_vmaster_hook() and put a barrier to assure the
hook_private_data store, too.


thanks,

Takashi