Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 18 Apr 2020 12:21:23 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     Alan Mikhak <alan.mikhak@sifive.com>
Cc:     linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
        linux-pci@vger.kernel.org, tglx@linutronix.de,
        gustavo.pimentel@synopsys.com, kishon@ti.com,
        paul.walmsley@sifive.com
Subject: Re: [PATCH] genirq/msi: Check null pointer before copying struct
 msi_msg
Message-ID: <20200418122123.10157ddd@why>
In-Reply-To: <1587149322-28104-1-git-send-email-alan.mikhak@sifive.com>
References: <1587149322-28104-1-git-send-email-alan.mikhak@sifive.com>
Organization: Approximate
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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

Hi Alan,

> From: Alan Mikhak <alan.mikhak@sifive.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@sifive.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...