Re: [PATCH 1/1] irq: Fix uaf issue in irq_find_at_or_after

[Thomas Gleixner]

On Thu, May 23 2024 at 19:39, dicken.ding wrote:
> The function "irq_find_at_or_after" is at the risk of use-after-free
> due to the race condition between the functions "delayer_free_desc"
> and "irq_desc_get_irq". The function "delayer_free_desc" could be
> called between "mt_find" and "irq_desc_get_irq" due to the absence
> of any locks to ensure atomic operations on the "irq_desc" structure.
>
> In this patch, we introduce a pair of locks, namely "rcu_read_lock"
> and "rcu_read_unlock" to prevent the occurrence of use-after-free in
> "irq_find_at_or_after".

Please read Documentation/process/maintainers-tip.rst and the general
documentation how changelogs should be written.

Something like this:

  irq_find_at_or_after() dereferences the interrupt descriptor which is
  returned by mt_find() while neither holding sparse_irq_lock nor RCU
  read lock, which means the descriptor can be freed between mt_find()
  and the dereference.

  Guard the access with a RCU read lock section.

Hmm?

> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -160,9 +160,15 @@ static int irq_find_free_area(unsigned int from, unsigned int cnt)
>  static unsigned int irq_find_at_or_after(unsigned int offset)
>  {
>  	unsigned long index = offset;
> +	unsigned int irq = nr_irqs;
> +
> +	rcu_read_lock();
>  	struct irq_desc *desc = mt_find(&sparse_irqs, &index, nr_irqs);
> +	if (desc)
> +		irq = irq_desc_get_irq(desc);
> +	rcu_read_unlock();
>  
> -	return desc ? irq_desc_get_irq(desc) : nr_irqs;
> +	return irq;

I wrote guard above because that's what should be used for this:

  	unsigned long index = offset;
  	struct irq_desc *desc;

        guard(rcu)();
        desc = mt_find(&sparse_irqs, &index, nr_irqs);
	return desc ? irq_desc_get_irq(desc) : nr_irqs;

Thanks,

        tglx