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

Date:   Mon, 21 Dec 2020 12:49:38 +0800
From:   Zenghui Yu <yuzenghui@...wei.com>
To:     Marc Zyngier <maz@...nel.org>
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 Marc,

On 2020/12/19 1:38, Marc Zyngier wrote:
> 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.

It seems that x86 will zero the whole msi_alloc_info_t structure at the
beginning of its .prepare (pci_msi_prepare()/init_irq_alloc_info()). If
this really affects something, I think we can easily address it with
some cleanup (on top of this patch).

> 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).

Indeed, thanks for fixing the commit message.

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

Thanks!


Zenghui