* Re: [PATCH] ALSA: pcm: Reject compat refinement after disconnect
2026-10-14 4:04 [PATCH] ALSA: pcm: Reject compat refinement after disconnect Cen Zhang
@ 2026-10-14 6:53 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2026-10-14 6:53 UTC (permalink / raw)
To: Cen Zhang
Cc: perex, tiwai, cassiogabrielcontato, kees, linux-sound,
linux-kernel, baijiaju1990, jjzuming
On Wed, 14 Oct 2026 06:04:12 +0200,
Cen Zhang wrote:
>
> A disconnected PCM stream must reject hardware refinement before
> entering its runtime constraint program. The native ioctl path checks
> for SNDRV_PCM_STATE_DISCONNECTED, but snd_pcm_ioctl_hw_params_compat()
> only checks that the runtime exists before handling HW_REFINE32. The
> built-in BUFFER_BYTES rule retains the substream as its private payload
> and reads substream->buffer_bytes_max.
>
> An open compat PCM file can therefore enter that rule after the
> component has been disconnected, while its owner is about to free the
> component. The request must include SNDRV_PCM_HW_PARAM_BUFFER_BYTES for
> this rule to run. A late HDA codec probe error on an already registered
> card can supply this teardown: snd_card_register() can publish an
> earlier PCM before a later registration fails, and
> snd_hda_codec_cleanup_for_unbind() then disconnects and frees the PCMs
> without the PCM-user drain used by normal codec removal.
>
> The failing order is:
>
> 1. The owner calls snd_pcm_dev_disconnect(), which marks the runtime
> DISCONNECTED and returns.
> 2. The compat file enters snd_pcm_ioctl_hw_params_compat() and then
> snd_pcm_hw_refine(), reaching snd_pcm_hw_rule_buffer_bytes_max().
> 3. The owner calls snd_device_free(); snd_pcm_free_stream() frees the
> substream while the rule callback is still in progress.
> 4. The callback reads buffer_bytes_max through its freed private
> payload, causing a use-after-free.
>
> The runtime and its rules remain allocated in this component-free path.
> The disconnect locks do not cover the subsequent free or the synchronous
> refinement loop, so they do not prevent this ordering.
>
> Reject compat HW_REFINE32 with -EBADFD when snd_pcm_get_state() reports
> a disconnected stream, matching the native ioctl. The existing helper
> reads the state under the stream lock. This stops refinement that starts
> after disconnect from entering the rule, closing the described window
> without changing component teardown.
>
> KASAN report as below:
>
> BUG: KASAN: slab-use-after-free in snd_pcm_hw_rule_buffer_bytes_max+0x276/0x2a0
> Read of size 8 at addr ffff888111b36870 by task pmbd_alsa_reade/486
>
> CPU: 2 UID: 0 PID: 486 Comm: pmbd_alsa_reade Tainted: G O 7.2.0-rc6-pmb-bt-functional-v1+ #1 PREEMPT(lazy)
> Tainted: [O]=OOT_MODULE
> 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
> Call Trace:
> <TASK>
> dump_stack_lvl+0x93/0xd0
> print_report+0xce/0x630
> ? snd_pcm_hw_rule_buffer_bytes_max+0x276/0x2a0
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __virt_addr_valid+0x20d/0x410
> ? snd_pcm_hw_rule_buffer_bytes_max+0x276/0x2a0
> kasan_report+0xe0/0x110
> ? snd_pcm_hw_rule_buffer_bytes_max+0x276/0x2a0
> snd_pcm_hw_rule_buffer_bytes_max+0x276/0x2a0
> ? snd_pcm_hw_rule_div+0x16b/0x200
> ? __pfx_snd_pcm_hw_rule_buffer_bytes_max+0x10/0x10
> ? constrain_params_by_rules+0x17a/0xfe0
> constrain_params_by_rules+0x506/0xfe0
> ? __pfx_constrain_params_by_rules+0x10/0x10
> ? __ia32_compat_sys_ioctl+0x289/0x2f0
> ? do_int80_emulation+0x14a/0x5e0
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? lock_release+0xc8/0x280
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? snd_interval_refine+0x284/0x470
> snd_pcm_hw_refine+0x768/0xcc0
> ? __pfx_snd_pcm_hw_refine+0x10/0x10
> ? _copy_from_user+0x51/0xa0
> snd_pcm_ioctl_hw_params_compat+0x138/0x280
> snd_pcm_ioctl_compat+0x4c5/0x7c0
> ? __pfx_snd_pcm_ioctl_compat+0x10/0x10
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? selinux_file_ioctl+0xaa/0x280
> ? ksys_write+0x16b/0x200
> ? __pfx_snd_pcm_ioctl_compat+0x10/0x10
> __ia32_compat_sys_ioctl+0x289/0x2f0
> ? srso_alias_return_thunk+0x5/0xfbef5
> do_int80_emulation+0x14a/0x5e0
> asm_int80_emulation+0x1a/0x20
> RIP: 0023:0x8049084
> Code: 80 b8 04 00 00 00 bb 01 00 00 00 b9 43 a0 04 08 ba 2b 00 00 00 cd 80 b8 36 00 00 00 89 f3 b9 10 41 5c c2 ba b8 b0 04 08 cd 80 <85> c0 78 6c b8 04 00 00 00 bb 01 00 00 00 b9 27 a0 04 08 ba 1c 00
> RSP: 002b:00000000ffd35e40 EFLAGS: 00000246 ORIG_RAX: 0000000000000036
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00000000c25c4110
> RDX: 000000000804b0b8 RSI: 0000000000000003 RDI: 0000000000000004
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> </TASK>
>
> Allocated by task 486:
> kasan_save_stack+0x33/0x60
> kasan_save_track+0x14/0x30
> __kasan_kmalloc+0xaa/0xb0
> __kmalloc_cache_noprof+0x251/0x610
> snd_pcm_new_stream+0x98f/0x1350
> _snd_pcm_new+0x1d1/0x310
> snd_pcm_new+0x43/0x60
> 0xffffffffc0401115
> do_one_initcall+0xf3/0x4e0
> do_init_module+0x281/0x8e0
> load_module+0x658c/0x7cc0
> init_module_from_file+0x15b/0x180
> idempotent_init_module+0x22d/0x770
> __x64_sys_finit_module+0xd2/0x160
> do_syscall_64+0x115/0x6a0
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Freed by task 494:
> kasan_save_stack+0x33/0x60
> kasan_save_track+0x14/0x30
> kasan_save_free_info+0x3b/0x60
> __kasan_slab_free+0x5f/0x80
> kfree+0x307/0x580
> snd_pcm_free_stream+0xcf/0x290
> snd_pcm_free+0xb1/0xf0
> __snd_device_free+0x1a1/0x490
> snd_device_free+0xcf/0x190
> case_owner_ioctl+0xb8/0x160 [pmbd_alsa_fixture]
> __x64_sys_ioctl+0x163/0x1d0
> do_syscall_64+0x115/0x6a0
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> The buggy address belongs to the object at ffff888111b36800
> which belongs to the cache kmalloc-1k of size 1024
> The buggy address is located 112 bytes inside of
> freed 1024-byte region [ffff888111b36800, ffff888111b36c00)
>
> The buggy address belongs to the physical page:
> page: refcount:0 mapcount:0 mapping:0000000000000000 index:0xffff888111b35800 pfn:0x111b30
> head: order:3 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> flags: 0x200000000000240(workingset|head|node=0|zone=2)
> page_type: f5(slab)
> raw: 0200000000000240 ffff888100042dc0 ffffea0004218610 ffffea00042c6810
> raw: ffff888111b35800 000000000010000f 00000000f5000000 0000000000000000
> head: 0200000000000240 ffff888100042dc0 ffffea0004218610 ffffea00042c6810
> head: ffff888111b35800 000000000010000f 00000000f5000000 0000000000000000
> head: 0200000000000003 fffffffffffffe01 00000000ffffffff 00000000ffffffff
> head: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888111b36700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff888111b36780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff888111b36800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff888111b36880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888111b36900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
Basically this isn't a problem only about the compat ioctl, but about
the all hw_refine-related ioctls. That is, snd_pcm_hw_refine() should
have the proper buffer locking and the state check like
snd_pcm_hw_params(). However, since it's called from many places that
are already under buffer lock, we need two functions (with and without
lock/state-check), and it'll be something like below.
Could you verify whether this works?
thanks,
Takashi
-- 8< --
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 359197aa6076..c54cb07304ad 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -252,7 +252,7 @@ static int snd_pcm_ioctl_hw_params_compat(struct snd_pcm_substream *substream,
return -EFAULT;
if (refine) {
- err = snd_pcm_hw_refine(substream, data);
+ err = snd_pcm_hw_refine_with_lock(substream, data);
if (err < 0)
return err;
err = fixup_unreferenced_params(substream, data);
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index a845f242a163..a6880a96a918 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -580,29 +580,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
}
EXPORT_SYMBOL(snd_pcm_hw_refine);
-static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params __user * _params)
-{
- int err;
- struct snd_pcm_hw_params *params __free(kfree) =
- memdup_user(_params, sizeof(*params));
-
- if (IS_ERR(params))
- return PTR_ERR(params);
-
- err = snd_pcm_hw_refine(substream, params);
- if (err < 0)
- return err;
-
- err = fixup_unreferenced_params(substream, params);
- if (err < 0)
- return err;
-
- if (copy_to_user(_params, params, sizeof(*params)))
- return -EFAULT;
- return 0;
-}
-
static int period_to_usecs(struct snd_pcm_runtime *runtime)
{
int usecs;
@@ -799,6 +776,22 @@ static void pcm_qos_req_update(struct snd_pcm_substream *substream, int usecs)
usecs);
}
+static int hw_params_state_check(struct snd_pcm_substream *substream)
+{
+ guard(pcm_stream_lock_irq)(substream);
+ switch (substream->runtime->state) {
+ case SNDRV_PCM_STATE_OPEN:
+ case SNDRV_PCM_STATE_SETUP:
+ case SNDRV_PCM_STATE_PREPARED:
+ if (!is_oss_stream(substream) &&
+ atomic_read(&substream->mmap_count))
+ return -EBADFD;
+ return 0;
+ default:
+ return -EBADFD;
+ }
+}
+
static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
@@ -813,20 +806,7 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
err = snd_pcm_buffer_access_lock(runtime);
if (err < 0)
return err;
- scoped_guard(pcm_stream_lock_irq, substream) {
- switch (runtime->state) {
- case SNDRV_PCM_STATE_OPEN:
- case SNDRV_PCM_STATE_SETUP:
- case SNDRV_PCM_STATE_PREPARED:
- if (!is_oss_stream(substream) &&
- atomic_read(&substream->mmap_count))
- err = -EBADFD;
- break;
- default:
- err = -EBADFD;
- break;
- }
- }
+ err = hw_params_state_check(substream);
if (err)
goto unlock;
@@ -947,6 +927,57 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream,
return err;
}
+/*
+ * call snd_pcm_hw_refine() after buffer lock and sanity check;
+ * this one should be used for ioctls and other external paths instead of
+ * direct call of snd_pcm_hw_refine()
+ */
+static int snd_pcm_hw_refine_with_lock(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params)
+{
+ struct snd_pcm_runtime *runtime;
+ int err;
+
+ if (PCM_RUNTIME_CHECK(substream))
+ return -ENXIO;
+ runtime = substream->runtime;
+ err = snd_pcm_buffer_access_lock(runtime);
+ if (err < 0)
+ return err;
+ err = hw_params_state_check(substream);
+ if (err)
+ goto unlock;
+
+ err = snd_pcm_hw_refine(substream, params);
+
+ unlock:
+ snd_pcm_buffer_access_unlock(runtime);
+ return err;
+}
+
+static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params __user * _params)
+{
+ int err;
+ struct snd_pcm_hw_params *params __free(kfree) =
+ memdup_user(_params, sizeof(*params));
+
+ if (IS_ERR(params))
+ return PTR_ERR(params);
+
+ err = snd_pcm_hw_refine_with_lock(substream, params);
+ if (err < 0)
+ return err;
+
+ err = fixup_unreferenced_params(substream, params);
+ if (err < 0)
+ return err;
+
+ if (copy_to_user(_params, params, sizeof(*params)))
+ return -EFAULT;
+ return 0;
+}
+
static int do_hw_free(struct snd_pcm_substream *substream)
{
int result = 0;
@@ -4218,7 +4249,7 @@ static int snd_pcm_hw_refine_old_user(struct snd_pcm_substream *substream,
if (IS_ERR(oparams))
return PTR_ERR(oparams);
snd_pcm_hw_convert_from_old_params(params, oparams);
- err = snd_pcm_hw_refine(substream, params);
+ err = snd_pcm_hw_refine_with_lock(substream, params);
if (err < 0)
return err;
^ permalink raw reply related [flat|nested] 2+ messages in thread