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

[Jiri Slaby <jirislaby@kernel.org>]

On 24. 06. 26, 3:21, 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.
> 
> 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>
> ---
> V4 -> V5:
> - Add __must_hold(&hash_mutex) annotation to
>    serial_get_or_create_irq_info() for static analysis.
> 
> 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.
> 
> v4: https://lore.kernel.org/r/20260529-bug-221579-8250-shared-irq-race-v4-1-cfda63b4420f@gmail.com
> v3: https://lore.kernel.org/r/20260529-bug-221579-8250-shared-irq-race-v3-1-fe4d430862a9@gmail.com
> 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 | 56 ++++++++++++++++++++++++++++---------
>   1 file changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index a428e88938eb7..a86505c3dcb67 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -131,10 +131,11 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
>    * - allocate a new one, add it to the hashtable and return it.
>    */
>   static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_port *up)
> +	__must_hold(&hash_mutex)
>   {
>   	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,31 +152,60 @@ 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);

So what is the reason to switch from guards to manual locking?

>   
> -		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);
> -	if (ret < 0)
> +	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);
> +	if (ret < 0) {
>   		serial_do_unlink(i, up);
> +		mutex_unlock(&hash_mutex);
> +		return ret;
> +	}
>   
> -	return ret;
> +	mutex_unlock(&hash_mutex);
> +
> +	return 0;
>   }
>   
>   static void serial_unlink_irq_chain(struct uart_8250_port *up)
> 
> ---
> base-commit: eb3f4b7426cfd2b79d65b7d37155480b32259a11
> change-id: 20260528-bug-221579-8250-shared-irq-race-581e4900a178
> 
> Best regards,


-- 
js
suse labs