linux-kernel - Re: [PATCH] genirq/msi: Initialize msi_alloc_info to zero for 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]

Date:   Fri, 18 Dec 2020 17:38:50 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Zenghui Yu <yuzenghui@...wei.com>
Cc:     <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <tglx@...utronix.de>,
        <kvm@...r.kernel.org>, <wanghaibin.wang@...wei.com>
Subject: Re: [PATCH] genirq/msi: Initialize msi_alloc_info to zero for msi_prepare API

Hi Zenghui,

On Fri, 18 Dec 2020 06:00:39 +0000,
Zenghui Yu <yuzenghui@...wei.com> wrote:
> 
> Since commit 5fe71d271df8 ("irqchip/gic-v3-its: Tag ITS device as shared if
> allocating for a proxy device"), some of the devices are wrongly marked as
> "shared" by the ITS driver on systems equipped with the ITS(es). The
> problem is that the @info->flags may not be initialized anywhere and we end
> up looking at random bits on the stack. That's obviously not good.
> 
> The straightforward fix is to properly initialize msi_alloc_info inside the
> .prepare callback of affected MSI domains (its-pci-msi, its-platform-msi,
> etc). We can also perform the initialization in IRQ core layer for
> msi_domain_prepare_irqs() API and it looks much neater to me.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@...wei.com>
> ---
> 
> This was noticed when I was playing with the assigned devices on arm64 and
> VFIO failed to enable MSI-X vectors for almost all VFs (CCed kvm list in
> case others will hit the same issue). It turned out that these VFs are
> marked as "shared" by mistake and have trouble with the following sequence:
> 
> 	pci_alloc_irq_vectors(pdev, 1, 1, flag);
> 	pci_free_irq_vectors(pdev);
> 	pci_alloc_irq_vectors(pdev, 1, 2, flag); --> we can only get
> 						     *one* vector
> 
> But besides VFIO, I guess there are already some devices get into trouble
> at probe time and can't work properly.
>
>  kernel/irq/msi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 2c0c4d6d0f83..dc0e2d7fbdfd 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -402,7 +402,7 @@ int __msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>  	struct msi_domain_ops *ops = info->ops;
>  	struct irq_data *irq_data;
>  	struct msi_desc *desc;
> -	msi_alloc_info_t arg;
> +	msi_alloc_info_t arg = { };
>  	int i, ret, virq;
>  	bool can_reserve;

Thanks for having investigated this. I guess my only worry with this
is that msi_alloc_info_t is a pretty large structure on x86, and
zeroing it isn't totally free. But this definitely looks nicer than
some of the alternatives (.prepare isn't a good option, as we do rely
on the flag being set in __platform_msi_create_device_domain(), which
calls itself .prepare).

I'll queue it, and we can always revisit this later if Thomas (or
anyone else) has a better idea.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.