[PATCH] ALSA: timer: Clear SNDRV_TIMER_IFLG_DEAD once the close completes

[PATCH] ALSA: timer: Clear SNDRV_TIMER_IFLG_DEAD once the close completes

[Norbert Szetei]

snd_timer_close_locked() marks an instance with SNDRV_TIMER_IFLG_DEAD
and returns early when the flag is already set, but the flag is never
cleared again.  A completed close ends in remove_slave_links(), which
leaves timeri->timer NULL, so a second close is already harmless through
the timer == NULL path; the early return can only be reached by an
instance that was opened again in between.  For such an instance the
close unlinks nothing, so snd_timer_instance_free() frees an object that
is still on timer->open_list_head, still on snd_timer_master_list if it
was opened with a slave key, still owns any adopted slaves, and still
holds its timer and module references.

snd_seq_timer_open() reopens an instance exactly like that: it retries
its fallback open on the same object after a failure that has already
run snd_timer_close_locked() internally.  An unprivileged user with
access to /dev/snd/timer and /dev/snd/seq can force that failure, since
snd_timer_check_master() returns -EBUSY when a pending slave matches the
new master's (slave_class, slave_id) key and the target timer has
reached max_instances, and SNDRV_TIMER_IOCTL_SELECT with dev_class =
SNDRV_TIMER_CLASS_SLAVE keeps the caller-supplied dev_sclass, so a
sequencer queue's key can be forged.  The freed instance is afterwards
dereferenced by any further snd_timer_open() on that timer, by
snd_timer_check_slave(), and by /proc/asound/timers, which faults on the
stale ti->owner pointer.

The flag only has to be visible while the close is in progress, which is
all its other users need.  Clear it in remove_slave_links(), under the
same timer->lock that sets it, once the instance is off every list.

Fixes: da3039e91d1f ("ALSA: timer: Forcibly close timer instances at closing")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Norbert Szetei <norbert@doyensec.com>
---
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 07ef127b29e1..9407954034cf 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -458,6 +458,8 @@ static void remove_slave_links(struct snd_timer_instance *timeri,
 		list_del_init(&slave->ack_list);
 		list_del_init(&slave->active_list);
 	}
+	/* the close is done; a reopen must not see the mark */
+	timeri->flags &= ~SNDRV_TIMER_IFLG_DEAD;
 }
 
 /*
-- 
2.55.0

Re: [PATCH] ALSA: timer: Clear SNDRV_TIMER_IFLG_DEAD once the close completes

[Takashi Iwai]

On Sun, 26 Jul 2026 10:01:45 +0200,
Norbert Szetei wrote:
> 
> snd_timer_close_locked() marks an instance with SNDRV_TIMER_IFLG_DEAD
> and returns early when the flag is already set, but the flag is never
> cleared again.  A completed close ends in remove_slave_links(), which
> leaves timeri->timer NULL, so a second close is already harmless through
> the timer == NULL path; the early return can only be reached by an
> instance that was opened again in between.  For such an instance the
> close unlinks nothing, so snd_timer_instance_free() frees an object that
> is still on timer->open_list_head, still on snd_timer_master_list if it
> was opened with a slave key, still owns any adopted slaves, and still
> holds its timer and module references.
> 
> snd_seq_timer_open() reopens an instance exactly like that: it retries
> its fallback open on the same object after a failure that has already
> run snd_timer_close_locked() internally.  An unprivileged user with
> access to /dev/snd/timer and /dev/snd/seq can force that failure, since
> snd_timer_check_master() returns -EBUSY when a pending slave matches the
> new master's (slave_class, slave_id) key and the target timer has
> reached max_instances, and SNDRV_TIMER_IOCTL_SELECT with dev_class =
> SNDRV_TIMER_CLASS_SLAVE keeps the caller-supplied dev_sclass, so a
> sequencer queue's key can be forged.  The freed instance is afterwards
> dereferenced by any further snd_timer_open() on that timer, by
> snd_timer_check_slave(), and by /proc/asound/timers, which faults on the
> stale ti->owner pointer.
> 
> The flag only has to be visible while the close is in progress, which is
> all its other users need.  Clear it in remove_slave_links(), under the
> same timer->lock that sets it, once the instance is off every list.
> 
> Fixes: da3039e91d1f ("ALSA: timer: Forcibly close timer instances at closing")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Norbert Szetei <norbert@doyensec.com>

Applied now.  Thanks.


Takashi