Re: [PATCH v19 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unvalidated PCI_COMMAND read can result in writing 0xFFFF back to the device, enabling fatal error reporting.
- [High] Blind restoration of PCI_COMMAND register clobbers concurrent modifications across the locked section (TOCTOU race).

Pre-existing issues:
- [High] Conditional enablement of PCI_COMMAND_MEMORY leaves pci_msix_write_vector_ctrl() exposed to Unsupported Request (UR) faults.
--

commit 9bf79a22431c8153c5aed81ed2a19f276bfd3940
Author: Farhan Ali <alifm@linux.ibm.com>

PCI/MSI: Enable memory decoding before restoring MSI-X messages

This commit attempts to fix Unsupported Request (UR) faults during MSI-X 
restoration by temporarily enabling the Memory bit in the Command register 
when writing MSI-X messages. The bit is enabled prior to restoration and 
restored to its previous state afterwards.

> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 81d24a270a795..d8d3c8a911ac0 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  {
>  	struct msi_desc *entry;
>  	bool write_msg;
> +	u16 cmd;
>  
>  	if (!dev->msix_enabled)
>  		return;

[ ... ]

> @@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
>  
>  	write_msg = arch_restore_msi_irqs(dev);
> +	if (write_msg) {
> +		pci_read_config_word(dev, PCI_COMMAND, &cmd);
> +		pci_write_config_word(dev, PCI_COMMAND,
> +				      cmd | PCI_COMMAND_MEMORY);
> +	}

[Severity: High]
Does pci_read_config_word() need its return value checked here?

If the read fails (e.g., due to a transient bus error or recovery state), the
PCI subsystem populates cmd with 0xFFFF (PCI_ERROR_RESPONSE). Computing
cmd | PCI_COMMAND_MEMORY would then write 0xFFFF back to the Command Register,
enabling all writable capabilities at once (including SERR# Enable), which
could trigger a fatal NMI or system panic.

>  	scoped_guard (msi_descs_lock, &dev->dev) {
>  		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
>  			if (write_msg)
>  				__pci_write_msi_msg(entry, &entry->msg);
>  			pci_msix_write_vector_ctrl(entry, entry->pci.msix_ctrl);

[Severity: High]
This is a pre-existing issue, but does this code leave the MSI-X table
exposed to the very Unsupported Request (UR) faults this patch tries to fix?

Since pci_msix_write_vector_ctrl() performs an MMIO write to the MSI-X
Vector Control register unconditionally for every MSI descriptor, what
happens if write_msg is false? The memory decoding bit won't be enabled
above, meaning this unconditional MMIO access could still trigger a UR fault.

>  		}
>  	}
>  
> +	if (write_msg)
> +		pci_write_config_word(dev, PCI_COMMAND, cmd);
> +

[Severity: High]
Can this blindly overwrite the PCI_COMMAND register with a stale snapshot?

The cmd value was read before the scoped_guard() block. If concurrent
operations modify PCI_COMMAND (such as changing PCI_COMMAND_MASTER or
PCI_COMMAND_INTX_DISABLE via sysfs or other subsystem operations) while
the lock is held, rewriting the old cmd value here would wipe out those
concurrent changes.

Should we re-read the register here, mask out PCI_COMMAND_MEMORY if we
enabled it, and only restore that specific bit?

>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615183524.2880-1-alifm@linux.ibm.com?part=4