linux-kernel - Re: [PATCH] genirq/msi: Check null pointer before copying struct msi

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200418122123.10157ddd@why>
Date:   Sat, 18 Apr 2020 12:21:23 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Alan Mikhak <alan.mikhak@...ive.com>
Cc:     linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-pci@...r.kernel.org, tglx@...utronix.de,
        gustavo.pimentel@...opsys.com, kishon@...com,
        paul.walmsley@...ive.com
Subject: Re: [PATCH] genirq/msi: Check null pointer before copying struct
 msi_msg

On Fri, 17 Apr 2020 11:48:42 -0700
Alan Mikhak <alan.mikhak@...ive.com> wrote:

Hi Alan,

> From: Alan Mikhak <alan.mikhak@...ive.com>
> 
> Modify __get_cached_msi_msg() to check both pointers for null before
> copying the contents from the struct msi_msg pointer to the pointer
> provided by caller.
> 
> Without this sanity check, __get_cached_msi_msg() crashes when invoked by
> dw_edma_irq_request() in drivers/dma/dw-edma/dw-edma-core.c running on a
> Linux-based PCIe endpoint device. MSI interrupt are not received by PCIe
> endpoint devices. As a result, irq_get_msi_desc() returns null since there
> are no cached struct msi_msg entry on the endpoint side.
> 
> Signed-off-by: Alan Mikhak <alan.mikhak@...ive.com>
> ---
>  kernel/irq/msi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index eb95f6106a1e..f39d42ef0d50 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -58,7 +58,8 @@ void free_msi_entry(struct msi_desc *entry)
>  
>  void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg)
>  {
> -	*msg = entry->msg;
> +	if (entry && msg)
> +		*msg = entry->msg;
>  }
>  
>  void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg)

I'm not convinced by this. If you know that, by construction, these
interrupts are not associated with an underlying MSI, why calling
get_cached_msi_msg() the first place?

There seem to be some assumptions in the DW EDMA driver that the
signaling would be MSI based, so maybe someone from Synopsys (Gustavo?)
could clarify that. From my own perspective, running on an endpoint
device means that it is *generating* interrupts, and I'm not sure what
the MSIs represent here.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...