linux-kernel - Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded

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:   Sun, 6 Feb 2022 15:34:56 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     lizhe <sensor1010@....com>, bhelgaas@...gle.com, lukas@...ner.de,
        ameynarkhede03@...il.com, naveennaidu479@...il.com
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with
 devm_request_threaded_irq

Hi Lihze,

On 2/6/22 15:07, lizhe wrote:
> Memory allocated with this function is automatically
> freed on driver detach
> 
> Signed-off-by: lizhe <sensor1010@....com>

You must use your real name (first-name + last-name) as author,
as well as in the Signed-off-by line, see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> ---
>  drivers/pci/hotplug/pciehp_hpc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 1c1ebf3dad43..aca59c6fdcbc 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -66,7 +66,7 @@ static inline int pciehp_request_irq(struct controller *ctrl)
>  	}
>  
>  	/* Installs the interrupt handler */
> -	retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist,
> +	retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
>  				      IRQF_SHARED, "pciehp", ctrl);
>  	if (retval)
>  		ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n",
> @@ -78,8 +78,6 @@ static inline void pciehp_free_irq(struct controller *ctrl)
>  {
>  	if (pciehp_poll_mode)
>  		kthread_stop(ctrl->poll_thread);
> -	else
> -		free_irq(ctrl->pcie->irq, ctrl);
>  }
>  
>  static int pcie_poll_cmd(struct controller *ctrl, int timeout)
> 

You cannot just go and change a single allocation into a devm managed
resource, esp. not a request_irq call.

Changing this into a devm_allocation means that the irq now will not be
free-ed until after pciehp_remove() has completed and that function calls:
pciehp_release_ctrl(ctrl); which releases the memory the ctrl pointer points
to and that same memory / pointer is used by pciehp_isr.

So after your patch it is possible for the IRQ to trigger after
pciehp_release_ctrl(ctrl) has free-ed the memory (and before the devm
framework calls free_irq() disabling the IRQ), causing pciehp_isr
to reference free-ed memory, leading to a crash.

Since this patch introduces a bug we cannot accept it, nack.

Regards,

Hans