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

[Greg Kroah-Hartman]

On Fri, May 29, 2026 at 04:23:34PM +0800, Qiliang Yuan wrote:
> 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 increments desc->depth,
> 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 (including the error path)
> so that no concurrent unlink or second-port join can race with IRQ
> setup or cleanup.
> serial_unlink_irq_chain() already holds hash_mutex throughout, so the
> race window is closed.

What real systems causes this to happen?  How are you triggering this
warning to happen?  How was this tested?

> 
> Fixes: 768aec0b5bcc ("serial: 8250: fix shared interrupts issues with SMP and RT kernels")
> Reported-by: Wang Zhaolong <wangzhaolong@xxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
> Signed-off-by: Qiliang Yuan <realwujing@xxxxxxxxx>
> ---
> V3 -> V4:
> - Move cleanup under hash_mutex on request_irq() failure to prevent a
>   second port from joining the chain before the irq_info is cleaned up.
> - Fix inaccurate description of irq_shutdown() in commit message.
> 
> 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.
> 
> v3: https://lore.kernel.org/r/20260529-bug-221579-8250-shared-irq-race-v3-1-fe4d430862a9@xxxxxxxxx
> v2: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v2-1-06531202e54d@xxxxxxxxx
> v1: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v1-1-30980cca02f3@xxxxxxxxx
> ---
>  drivers/tty/serial/8250/8250_core.c | 55 ++++++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index a428e88938eb7..70d5acfa591bf 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);

Shouldn't the function be marked as requiring this lock to be held?
Just putting in this lockdep_assert will not catch the static analysis
tools :(

thanks,

greg k-h