linux-kernel - Re: [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked for update

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:   Wed, 21 Jul 2021 15:32:38 -0700
From:   "Raj, Ashok" <ashok.raj@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Kevin Tian <kevin.tian@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Marc Zyngier <maz@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        x86@...nel.org, Ashok Raj <ashok.raj@...el.com>
Subject: Re: [patch 3/8] PCI/MSI: Enforce that MSI-X table entry is masked
 for update

On Wed, Jul 21, 2021 at 09:11:29PM +0200, Thomas Gleixner wrote:
> The specification states:
> 
>     For MSI-X, a function is permitted to cache Address and Data values
>     from unmasked MSI-X Table entries. However, anytime software unmasks a
>     currently masked MSI-X Table entry either by clearing its Mask bit or
>     by clearing the Function Mask bit, the function must update any Address
>     or Data values that it cached from that entry. If software changes the
>     Address or Data value of an entry while the entry is unmasked, the
>     result is undefined.
> 
> The Linux kernel's MSI-X support never enforced that the entry is masked
> before the entry is modified hence the Fixes tag refers to a commit in:
>       git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
> 
> Enforce the entry to be masked across the update.
> 
> There is no point in enforcing this to be handled at all possible call
> sites as this is just pointless code duplication and the common update
> function is the obvious place to enforce this.
> 
> Reported-by: Kevin Tian <kevin.tian@...el.com>
> Fixes: f036d4ea5fa7 ("[PATCH] ia32 Message Signalled Interrupt support")
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> Cc: linux-pci@...r.kernel.org
> ---
>  drivers/pci/msi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -289,13 +289,28 @@ void __pci_write_msi_msg(struct msi_desc
>  		/* Don't touch the hardware now */
>  	} else if (entry->msi_attrib.is_msix) {
>  		void __iomem *base = pci_msix_desc_addr(entry);
> +		bool unmasked = !(entry->masked & PCI_MSIX_ENTRY_CTRL_MASKBIT);
>  
>  		if (!base)
>  			goto skip;
>  
> +		/*
> +		 * The specification mandates that the entry is masked
> +		 * when the message is modified:
> +		 *
> +		 * "If software changes the Address or Data value of an
> +		 * entry while the entry is unmasked, the result is
> +		 * undefined."
> +		 */
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, PCI_MSIX_ENTRY_CTRL_MASKBIT);
> +

Is there any locking needs here? say during cpu hotplug and some user-space
setting affinity?

>  		writel(msg->address_lo, base + PCI_MSIX_ENTRY_LOWER_ADDR);
>  		writel(msg->address_hi, base + PCI_MSIX_ENTRY_UPPER_ADDR);
>  		writel(msg->data, base + PCI_MSIX_ENTRY_DATA);
> +
> +		if (unmasked)
> +			__pci_msix_desc_mask_irq(entry, 0);
>  	} else {
>  		int pos = dev->msi_cap;
>  		u16 msgctl;
>