Received-SPF: pass (google.com: domain of linux-kernel+bounces-187941-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: "dicken.ding" <dicken.ding@mediatek.com>, Matthias Brugger
 <matthias.bgg@gmail.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@collabora.com>
Cc: wsd_upstream@mediatek.com, hanks.chen@mediatek.com,
 ivan.tseng@mediatek.com, cheng-jui.wang@mediatek.com,
 linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-mediatek@lists.infradead.org, "dicken.ding"
 <dicken.ding@mediatek.com>
Subject: Re: [PATCH 1/1] irq: Fix uaf issue in irq_find_at_or_after
In-Reply-To: <20240523113949.10444-1-dicken.ding@mediatek.com>
Date: Thu, 23 May 2024 21:46:36 +0200
Message-ID: <87cypcfh0j.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

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