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

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

[Qiliang Yuan]

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.

Dropping hash_mutex before request_irq() completes also allows another
port sharing the same IRQ to join the chain and run the shared-IRQ THRE
test while IRQ startup is still in progress, which can also trigger the
"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() and holding it
across the first request_irq() completion so that no concurrent
unlink or second-port join can race with IRQ setup.
serial_unlink_irq_chain() already holds hash_mutex throughout, so the
race window is closed.

Fixes: 768aec0b5bcc ("serial: 8250: fix shared interrupts issues with SMP and RT kernels")
Reported-by: Wang Zhaolong <wangzhaolong@fnnas.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Signed-off-by: Qiliang Yuan <realwujing@gmail.com>
---
V2 -> V3:
- Hold hash_mutex across the first request_irq() completion to prevent a
  second port from joining the chain and running the shared-IRQ THRE test
  while IRQ startup is still in progress.

V1 -> V2:
- Add Reported-by tag from Wang Zhaolong.

v2: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v2-1-06531202e54d@gmail.com
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 | 49 ++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb7..55a1a0515397f 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,54 @@ 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 from the hash lookup through the first
+ * request_irq() completion.  Dropping it earlier allows a concurrent
+ * serial_unlink_irq_chain() to race in after i->head is published but
+ * before the IRQ is fully set up — another port sharing the IRQ can then
+ * join the chain and run the shared-IRQ THRE test while IRQ startup is
+ * still in progress, triggering 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 entering and freeing the
+	 * irq_info until the first request_irq() completes.
+	 */
+	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);
+
+	ret = request_irq(up->port.irq, serial8250_interrupt,
+			  up->port.irqflags, up->port.name, i);
+
+	mutex_unlock(&hash_mutex);
+
 	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 v3] serial: 8250: fix use-after-free in IRQ chain handling

[Wang Zhaolong]

v3 fixes the Bugzilla reproducer on my setup.

But this error path is still racy:

> +
> +	ret = request_irq(up->port.irq, serial8250_interrupt,
> +			  up->port.irqflags, up->port.name, i);
> +
> +	mutex_unlock(&hash_mutex);
> +
>  	if (ret < 0)
>  		serial_do_unlink(i, up);
>  
> 

i is already in irq_lists and i->head is already visible here.  On
request_irq() failure, another port can join the chain and return success
without any IRQ handler installed.

The cleanup must happen before dropping hash_mutex.

> Dropping hash_mutex before request_irq() completes also allows another
> port sharing the same IRQ to join the chain and run the shared-IRQ THRE
> test while IRQ startup is still in progress, which can also trigger the
> "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().

The changelog is also still inaccurate: irq_shutdown() does not hard-set
desc->depth to 1 on current mainline; it increments desc->depth.

Best regards,
Wang Zhaolong