ALSA: iterator used after loop end in timer/seq port registration?

ALSA: iterator used after loop end in timer/seq port registration?

[Maoyi Xie]

Hi all,

While reading sound/core/ I noticed two places where the list
iterator is used after the loop has walked past the end of the
list. I would appreciate it if you could take a look and let me
know whether these are real issues, and whether they are worth
fixing.

The first is snd_timer_dev_register() in sound/core/timer.c
(linux-7.1-rc1, around line 1018):

    list_for_each_entry(timer1, &snd_timer_list, device_list) {
            if (timer1->tmr_class > timer->tmr_class)
                    break;
            ...
            return -EBUSY;
    }
    list_add_tail(&timer->device_list, &timer1->device_list);

The second is snd_seq_create_port() in sound/core/seq/seq_ports.c
(linux-7.1-rc1, around line 147):

    list_for_each_entry(p, &client->ports_list_head, list) {
            ...
            if (p->addr.port > num)
                    break;
            ...
    }
    list_add_tail(&new_port->list, &p->list);

In both cases, when the loop walks all entries without break, the
iterator has gone one step past the last entry. &iter->member
then aliases the list head via container_of offset cancellation,
so the insert lands at the list tail. That is the intended
behaviour, but the access is undefined per C11.

Jakob Koschel cleaned up many such sites in 2022, for example
commits 99d8ae4ec8a (tracing: Remove usage of list iterator
variable after the loop), 2966a9918df (clockevents: Use dedicated
list iterator variable) and dc1acd5c946 (dlm: replace usage of
found with dedicated list iterator variable). The two sites in
sound/core/ were not covered.

A candidate fix would track an explicit insert_before pointer
initialised to the list head and overwritten to &iter->member only
when the loop breaks early. The observable behaviour is unchanged.

If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix to
you. Thank you for your time, and sorry for the noise if this is
not actually worth fixing or has already been spotted.

Thanks,
Maoyi Xie
https://maoyixie.com/

Re: ALSA: iterator used after loop end in timer/seq port registration?

[Takashi Iwai]

On Mon, 18 May 2026 18:00:29 +0200,
Maoyi Xie wrote:
> 
> Hi all,
> 
> While reading sound/core/ I noticed two places where the list
> iterator is used after the loop has walked past the end of the
> list. I would appreciate it if you could take a look and let me
> know whether these are real issues, and whether they are worth
> fixing.
> 
> The first is snd_timer_dev_register() in sound/core/timer.c
> (linux-7.1-rc1, around line 1018):
> 
>     list_for_each_entry(timer1, &snd_timer_list, device_list) {
>             if (timer1->tmr_class > timer->tmr_class)
>                     break;
>             ...
>             return -EBUSY;
>     }
>     list_add_tail(&timer->device_list, &timer1->device_list);
> 
> The second is snd_seq_create_port() in sound/core/seq/seq_ports.c
> (linux-7.1-rc1, around line 147):
> 
>     list_for_each_entry(p, &client->ports_list_head, list) {
>             ...
>             if (p->addr.port > num)
>                     break;
>             ...
>     }
>     list_add_tail(&new_port->list, &p->list);
> 
> In both cases, when the loop walks all entries without break, the
> iterator has gone one step past the last entry. &iter->member
> then aliases the list head via container_of offset cancellation,
> so the insert lands at the list tail. That is the intended
> behaviour, but the access is undefined per C11.
> 
> Jakob Koschel cleaned up many such sites in 2022, for example
> commits 99d8ae4ec8a (tracing: Remove usage of list iterator
> variable after the loop), 2966a9918df (clockevents: Use dedicated
> list iterator variable) and dc1acd5c946 (dlm: replace usage of
> found with dedicated list iterator variable). The two sites in
> sound/core/ were not covered.
> 
> A candidate fix would track an explicit insert_before pointer
> initialised to the list head and overwritten to &iter->member only
> when the loop breaks early. The observable behaviour is unchanged.
> 
> If this is intentional or already known, please disregard.
> Otherwise, I am happy to send a [PATCH] or to leave the fix to
> you. Thank you for your time, and sorry for the noise if this is
> not actually worth fixing or has already been spotted.

Thanks for the report.  I believe those two are actually bugs,
likely introduced by the early commit 9244b2c3079f
    [ALSA] alsa core: convert to list_for_each_entry*

The original code were with list_for_each(), and at the end of the
loop, it was supposed to go back to the list head.

If you have already fix patches ready for submission, it'd be
appreciated.  Otherwise I'll fix them up quickly, too.


Takashi

[PATCH 0/2] ALSA: avoid past-the-end iterators in timer/seq port registration

[Maoyi Xie]

Two fixes in sound/core that remove past-the-end iterator
use of the shape

    list_for_each_entry(iter, head, member) {
        if (...) break;
    }
    list_add_tail(&new, &iter->member);

When the loop walks all entries without break, iter is past
the end and &iter->member aliases the list head via
container_of offset cancellation. The insert lands at the
list tail, which is the intended behaviour, but the access
is undefined per C11.

Takashi Iwai confirmed on the inquiry thread that both sites
are bugs introduced by commit 9244b2c3079f ("[ALSA] alsa core:
convert to list_for_each_entry*"). The original list_for_each()
loop terminated at the list head; the conversion to
list_for_each_entry() left the post-loop access using the
container struct, which aliases the head via offset
cancellation.

The patched code tracks an explicit insert_before pointer
initialised to the list head and overwritten to &iter->member
only when the loop breaks early. Observable behaviour is
unchanged.

Inquiry thread:
https://lore.kernel.org/linux-sound/?q=iterator+used+after+loop+end+in+timer

Maoyi Xie (2):
  ALSA: timer: avoid past-the-end iterator in snd_timer_dev_register()
  ALSA: seq: avoid past-the-end iterator in snd_seq_create_port()

 sound/core/seq/seq_ports.c |  7 +++++--
 sound/core/timer.c         | 19 ++++++++++++++-----
 2 files changed, 19 insertions(+), 7 deletions(-)

[PATCH 1/2] ALSA: timer: avoid past-the-end iterator in snd_timer_dev_register()

[Maoyi Xie]

snd_timer_dev_register() walks snd_timer_list looking for the
ordered insertion point and on loop fall-through passes
&timer1->device_list to list_add_tail():

    list_for_each_entry(timer1, &snd_timer_list, device_list) {
            ...
            break;        /* on found-position */
            ...
    }
    list_add_tail(&timer->device_list, &timer1->device_list);

When the loop walks all entries without break, timer1 is
past-the-end. &timer1->device_list aliases &snd_timer_list (the
list head) via container_of offset cancellation, so the insert
lands at the list tail. That is the intended behaviour, but the
access is undefined per C11 even though it works in practice.

Track an explicit insert_before pointer initialised to the list
head and overwritten to &timer1->device_list only when the loop
breaks early. The observable behaviour is unchanged.

Fixes: 9244b2c3079f ("[ALSA] alsa core: convert to list_for_each_entry*")
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 sound/core/timer.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

--- a/sound/core/timer.c	2026-05-18 19:17:08.274971549 +0800
+++ b/sound/core/timer.c	2026-05-18 19:17:46.598676455 +0800
@@ -1007,6 +1007,7 @@
 {
 	struct snd_timer *timer = dev->device_data;
 	struct snd_timer *timer1;
+	struct list_head *insert_before = &snd_timer_list;
 
 	if (snd_BUG_ON(!timer || !timer->hw.start || !timer->hw.stop))
 		return -ENXIO;
@@ -1016,28 +1017,36 @@
 
 	guard(mutex)(&register_mutex);
 	list_for_each_entry(timer1, &snd_timer_list, device_list) {
-		if (timer1->tmr_class > timer->tmr_class)
+		if (timer1->tmr_class > timer->tmr_class) {
+			insert_before = &timer1->device_list;
 			break;
+		}
 		if (timer1->tmr_class < timer->tmr_class)
 			continue;
 		if (timer1->card && timer->card) {
-			if (timer1->card->number > timer->card->number)
+			if (timer1->card->number > timer->card->number) {
+				insert_before = &timer1->device_list;
 				break;
+			}
 			if (timer1->card->number < timer->card->number)
 				continue;
 		}
-		if (timer1->tmr_device > timer->tmr_device)
+		if (timer1->tmr_device > timer->tmr_device) {
+			insert_before = &timer1->device_list;
 			break;
+		}
 		if (timer1->tmr_device < timer->tmr_device)
 			continue;
-		if (timer1->tmr_subdevice > timer->tmr_subdevice)
+		if (timer1->tmr_subdevice > timer->tmr_subdevice) {
+			insert_before = &timer1->device_list;
 			break;
+		}
 		if (timer1->tmr_subdevice < timer->tmr_subdevice)
 			continue;
 		/* conflicts.. */
 		return -EBUSY;
 	}
-	list_add_tail(&timer->device_list, &timer1->device_list);
+	list_add_tail(&timer->device_list, insert_before);
 	return 0;
 }
 
-- 
2.34.1

[PATCH 2/2] ALSA: seq: avoid past-the-end iterator in snd_seq_create_port()

[Maoyi Xie]

snd_seq_create_port() walks client->ports_list_head looking for
the ordered insertion point and on loop fall-through passes
&p->list to list_add_tail():

    list_for_each_entry(p, &client->ports_list_head, list) {
            if (p->addr.port == port) {
                    kfree(new_port);
                    return -EBUSY;
            }
            if (p->addr.port > num)
                    break;
            ...
    }
    list_add_tail(&new_port->list, &p->list);

When the loop walks all entries without break (e.g., the new
port sorts last), p is past-the-end. &p->list aliases
&client->ports_list_head (the list head) via container_of offset
cancellation, so the insert lands at the list tail. That is the
intended behaviour, but the access is undefined per C11 even
though it works in practice.

Track an explicit insert_before pointer initialised to the list
head and overwritten to &p->list only when the loop breaks
early. The observable behaviour is unchanged.

Fixes: 9244b2c3079f ("[ALSA] alsa core: convert to list_for_each_entry*")
Signed-off-by: Maoyi Xie <maoyixie.tju@gmail.com>
---
 sound/core/seq/seq_ports.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- a/sound/core/seq/seq_ports.c	2026-05-18 19:17:08.278971518 +0800
+++ b/sound/core/seq/seq_ports.c	2026-05-18 19:18:44.006234339 +0800
@@ -144,18 +144,21 @@
 	num = max(port, 0);
 	guard(mutex)(&client->ports_mutex);
 	guard(write_lock_irq)(&client->ports_lock);
+	struct list_head *insert_before = &client->ports_list_head;
 	list_for_each_entry(p, &client->ports_list_head, list) {
 		if (p->addr.port == port) {
 			kfree(new_port);
 			return -EBUSY;
 		}
-		if (p->addr.port > num)
+		if (p->addr.port > num) {
+			insert_before = &p->list;
 			break;
+		}
 		if (port < 0) /* auto-probe mode */
 			num = p->addr.port + 1;
 	}
 	/* insert the new port */
-	list_add_tail(&new_port->list, &p->list);
+	list_add_tail(&new_port->list, insert_before);
 	client->num_ports++;
 	new_port->addr.port = num;	/* store the port number in the port */
 	sprintf(new_port->name, "port-%d", num);
-- 
2.34.1

Re: [PATCH 0/2] ALSA: avoid past-the-end iterators in timer/seq port registration

[Takashi Iwai]

On Mon, 18 May 2026 21:40:21 +0200,
Maoyi Xie wrote:
> 
> Two fixes in sound/core that remove past-the-end iterator
> use of the shape
> 
>     list_for_each_entry(iter, head, member) {
>         if (...) break;
>     }
>     list_add_tail(&new, &iter->member);
> 
> When the loop walks all entries without break, iter is past
> the end and &iter->member aliases the list head via
> container_of offset cancellation. The insert lands at the
> list tail, which is the intended behaviour, but the access
> is undefined per C11.
> 
> Takashi Iwai confirmed on the inquiry thread that both sites
> are bugs introduced by commit 9244b2c3079f ("[ALSA] alsa core:
> convert to list_for_each_entry*"). The original list_for_each()
> loop terminated at the list head; the conversion to
> list_for_each_entry() left the post-loop access using the
> container struct, which aliases the head via offset
> cancellation.
> 
> The patched code tracks an explicit insert_before pointer
> initialised to the list head and overwritten to &iter->member
> only when the loop breaks early. Observable behaviour is
> unchanged.
> 
> Inquiry thread:
> https://lore.kernel.org/linux-sound/?q=iterator+used+after+loop+end+in+timer
> 
> Maoyi Xie (2):
>   ALSA: timer: avoid past-the-end iterator in snd_timer_dev_register()
>   ALSA: seq: avoid past-the-end iterator in snd_seq_create_port()

Applied both patches now.  Thanks.


Takashi