linux-kernel - Re: [PATCH 1/1] irq: Fix uaf issue in irq_find_at_or

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 23 May 2024 21:46:36 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: "dicken.ding" <dicken.ding@...iatek.com>, Matthias Brugger
 <matthias.bgg@...il.com>, AngeloGioacchino Del Regno
 <angelogioacchino.delregno@...labora.com>
Cc: wsd_upstream@...iatek.com, hanks.chen@...iatek.com,
 ivan.tseng@...iatek.com, cheng-jui.wang@...iatek.com,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, "dicken.ding"
 <dicken.ding@...iatek.com>
Subject: Re: [PATCH 1/1] irq: Fix uaf issue in irq_find_at_or_after

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