[PATCH v2] serial: 8250: fix use-after-free in IRQ chain handling

Qiliang Yuan posted 1 patch 1 week, 3 days ago
There is a newer version of this series
drivers/tty/serial/8250/8250_core.c | 46 ++++++++++++++++++++++++++++---------
1 file changed, 35 insertions(+), 11 deletions(-)
[PATCH v2] serial: 8250: fix use-after-free in IRQ chain handling
Posted by Qiliang Yuan 1 week, 3 days ago
serial_unlink_irq_chain() holds hash_mutex and calls free_irq() + kfree(i)
when it sees an empty port list.  serial_link_irq_chain() released
hash_mutex after serial_get_or_create_irq_info() but before acquiring
i->lock.  This gap allowed a concurrent unlink to observe list_empty()
as true while a new port was still being added, free i, and trigger a
use-after-free.

The corrupted list/irq_info then causes an "Unbalanced enable for IRQ"
warning (kernel/irq/manage.c:774) because irq_shutdown() in the premature
free_irq() path hard-sets desc->depth to 1, breaking the disable_irq/
enable_irq pairing in serial8250_THRE_test().

Fix by pulling hash_mutex into serial_link_irq_chain() so that it covers
the i->head check and the list_add/INIT_LIST_HEAD under i->lock.
serial_unlink_irq_chain() already holds hash_mutex throughout, so the
race window is closed.

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Reported-by: Wang Zhaolong <wangzhaolong@fnnas.com>
Fixes: 768aec0b5bcc ("serial: 8250: fix shared interrupts issues with SMP and RT kernels")
Signed-off-by: Qiliang Yuan <realwujing@gmail.com>
---
Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v1-1-30980cca02f3@gmail.com
---
 drivers/tty/serial/8250/8250_core.c | 46 ++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb7..dfe76223ce10c 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -134,7 +134,7 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
 {
 	struct irq_info *i;
 
-	guard(mutex)(&hash_mutex);
+	lockdep_assert_held(&hash_mutex);
 
 	hash_for_each_possible(irq_lists, i, node, up->port.irq)
 		if (i->irq == up->port.irq)
@@ -151,27 +151,51 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
 	return i;
 }
 
+/*
+ * serial_link_irq_chain() hooks the given 8250 port into the IRQ chain.
+ *
+ * hash_mutex must be held across checking i->head and adding the port to
+ * the list.  Without this, a concurrent serial_unlink_irq_chain() can race
+ * in after hash_mutex is dropped but before i->lock is acquired, observe
+ * list_empty(i->head) as true, call free_irq() and kfree(i) — triggering a
+ * use-after-free and ultimately an "Unbalanced enable for IRQ" warning in
+ * kernel/irq/manage.c.
+ */
 static int serial_link_irq_chain(struct uart_8250_port *up)
 {
 	struct irq_info *i;
 	int ret;
 
+	mutex_lock(&hash_mutex);
+
 	i = serial_get_or_create_irq_info(up);
-	if (IS_ERR(i))
+	if (IS_ERR(i)) {
+		mutex_unlock(&hash_mutex);
 		return PTR_ERR(i);
+	}
 
-	scoped_guard(spinlock_irq, &i->lock) {
-		if (i->head) {
-			list_add(&up->list, i->head);
-
-			return 0;
-		}
+	/*
+	 * Serialise against the list manipulation in the interrupt handler
+	 * and in serial_unlink_irq_chain().  hash_mutex is still held which
+	 * prevents serial_unlink_irq_chain() from running concurrently.
+	 */
+	spin_lock_irq(&i->lock);
+	if (i->head) {
+		list_add(&up->list, i->head);
+		spin_unlock_irq(&i->lock);
+		mutex_unlock(&hash_mutex);
 
-		INIT_LIST_HEAD(&up->list);
-		i->head = &up->list;
+		return 0;
 	}
 
-	ret = request_irq(up->port.irq, serial8250_interrupt, up->port.irqflags, up->port.name, i);
+	INIT_LIST_HEAD(&up->list);
+	i->head = &up->list;
+	spin_unlock_irq(&i->lock);
+
+	mutex_unlock(&hash_mutex);
+
+	ret = request_irq(up->port.irq, serial8250_interrupt,
+			  up->port.irqflags, up->port.name, i);
 	if (ret < 0)
 		serial_do_unlink(i, up);
 

---
base-commit: eb3f4b7426cfd2b79d65b7d37155480b32259a11
change-id: 20260528-bug-221579-8250-shared-irq-race-581e4900a178

Best regards,
-- 
Qiliang Yuan <realwujing@gmail.com>

Re: [PATCH v2] serial: 8250: fix use-after-free in IRQ chain handling
Posted by 王曌龙 1 week, 3 days ago
Hi Qiliang

> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579

This patch does not fix the reported issue.

I tested it with the Bugzilla reproducer, and the "Unbalanced enable for IRQ"
warning is still reproducible.

```
chmod +x /tmp/run-tty-open-race.sh
/tmp/run-tty-open-race.sh
racing simultaneous open/close on /dev/ttyS1 and /dev/ttyS3, 100000 iterations
[   61.937561][  T915] ------------[ cut here ]------------
[   61.938276][  T915] Unbalanced enable for IRQ 3
[   61.938828][  T915] WARNING: kernel/irq/manage.c:774 at __enable_irq+0x33/0x60, CPU#1: tty-open-race/915
[   61.939978][  T915] Modules linked in: af_packet(E) kvm_amd(E) nfnetlink(E) ahci(E) libahci(E) libata(E) lpc_ich(E) virtio_net(E) net_failover(E) failover(E) sunrpc(E) dm_mod(E) raid1(E) binfmt_misc(E) md_mod(E) 9p(E) 9pnet_virtio(E) 9pnet(E) ext4(E) netfs(E) virtio_scsi(E) scsi_mod(E) crc16(E) mbcache(E) jbd2(E) virtio_blk(E) scsi_common(E) virtiofs(E) fuse(E) vmw_vsock_virtio_transport(E) vmw_vsock_virtio_transport_common(E) vsock(E)
[   61.944542][  T915] CPU: 1 UID: 0 PID: 915 Comm: tty-open-race Tainted: G            E       7.1.0-rc5-trim+ #14 PREEMPT(full)  43dd8c1eb10e193a5538796ee5a4e7f9cf3e86f3
[   61.946304][  T915] Tainted: [E]=UNSIGNED_MODULE
[   61.946866][  T915] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS Arch Linux 1.17.0-2-2 04/01/2014
[   61.948045][  T915] RIP: 0010:__enable_irq+0x39/0x60
[   61.948650][  T915] Code: 85 c0 74 19 83 f8 01 74 0e 83 e8 01 89 87 80 00 00 00 e9 d5 52 c8 ff f6 47 7d 08 74 17 48 8d 05 ed 01 2c 02 8b 77 2c 48 89 c7 <67> 48 0f b9 3a e9 b8 52 c8 ff 81 4f 78 00 04 00 00 ba 01 00 00 00
[   61.950925][  T915] RSP: 0018:ffffc90000f67858 EFLAGS: 00010046
[   61.951634][  T915] RAX: ffffffff8373f030 RBX: 0000000000000001 RCX: ffff888108912380
[   61.952574][  T915] RDX: 0000000000000001 RSI: 0000000000000003 RDI: ffffffff8373f030
[   61.953508][  T915] RBP: 0000000000000003 R08: ffff888108912300 R09: 0000000000000001
[   61.954438][  T915] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000246
[   61.955367][  T915] R13: ffff8881242ef000 R14: 0000000000000001 R15: 0000000000000002
[   61.956309][  T915] FS:  00007fba5a6106c0(0000) GS:ffff8881eec34000(0000) knlGS:0000000000000000
[   61.957351][  T915] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   61.958122][  T915] CR2: 000000001d2b1258 CR3: 000000010e5cf000 CR4: 0000000000750ef0
[   61.959056][  T915] PKRU: 55555554
[   61.959501][  T915] Call Trace:
[   61.959913][  T915]  <TASK>
[   61.960262][  T915]  enable_irq+0x7e/0x100
[   61.960769][  T915]  serial8250_do_startup+0x7ce/0xa80
[   61.961395][  T915]  uart_port_startup+0x13d/0x440
[   61.961981][  T915]  uart_port_activate+0x5b/0xb0
[   61.962551][  T915]  tty_port_open+0x90/0x110
[   61.963089][  T915]  ? srso_alias_return_thunk+0x5/0xfbef5
[   61.963747][  T915]  uart_open+0x1e/0x30
```

The problem is that the first-port path still drops hash_mutex before
request_irq() completes:

  i->head = &up->list;
  mutex_unlock(&hash_mutex);
  request_irq(..., i);

After i->head is published, another port sharing the IRQ can still join the
chain and run the shared-IRQ THRE test while the IRQ core is starting the
interrupt for the first port.

So the lock has to cover the first request_irq() completion.  Covering only
the i->head check and list_add() is not sufficient.

Test result: still fails with the Bugzilla reproducer.

Thanks,
Wang Zhaolong
Re: [PATCH v2] serial: 8250: fix use-after-free in IRQ chain handling
Posted by Wang Zhaolong 1 week, 3 days ago
My patch should cover that link/unlink lifetime race as well.

serial_unlink_irq_chain() already holds hash_mutex before walking irq_lists
and before calling serial_do_unlink(), which can kfree(i).  My patch keeps
hash_mutex held in serial_link_irq_chain() from serial_get_or_create_irq_info()
through the list update and the first request_irq() completion.

So once serial_link_irq_chain() has found or allocated i, a concurrent unlink
cannot enter serial_unlink_irq_chain() and free that irq_info until the link
path is done.

The important difference is that the lock is also held across the first
request_irq().  That is required for the reported "Unbalanced enable for IRQ"
race, because publishing i->head before request_irq() completes lets another
port join the chain and run the shared-IRQ THRE test while IRQ startup is still
in progress.

Thanks,
Wang Zhaolong