Re: [PATCH v2 06/13] PCI: Account for ACS egress control in isolation checks

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

Pre-existing issues:
- [High] Unchecked PCI configuration reads in Intel SPT PCH ACS quirks lead to hardware state corruption on read failures.
--

--- Patch [6]: [PATCH v2 06/13] PCI: Account for ACS egress control in isolation checks ---
commit 99c1da7ad98e7bc62ca4515768fee9a8109e4739
Author: Leon Romanovsky <[email protected]>

PCI: Account for ACS egress control in isolation checks

This commit modifies ACS isolation checks to ensure peer requests cannot
bypass the IOMMU when Egress Control overrides Request Redirect. It also
updates the Intel SPT PCH quirk to gracefully handle configuration read
failures and properly enforce isolation rules.

[ ... ]

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index bb4c09cbbd10d..ebfe902b01182 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4990,15 +4990,21 @@ static int pci_quirk_intel_spt_pch_acs(struct pci_dev *dev, u16 acs_flags)
>  		return -ENOTTY;
>  
>  	/* see pci_acs_flags_enabled() */
> -	pci_read_config_dword(dev, pos + PCI_ACS_CAP, &cap);
> +	if (pci_read_config_dword(dev, pos + PCI_ACS_CAP, &cap))
> +		return 0;
>  	acs_flags &= (cap | PCI_ACS_EC);
>  
> -	pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl);
> +	if (pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl))
> +		return 0;

[Severity: High]
This is a pre-existing issue, but since this patch adds return checks for
pci_read_config_dword() here, should similar checks be added to
pci_quirk_enable_intel_spt_pch_acs() and
pci_quirk_disable_intel_spt_pch_acs_redir() in the same file?

If pci_read_config_dword() fails in those functions, cap and ctrl are
populated with 0xFFFFFFFF by PCI_SET_ERROR_RESPONSE(). The subsequent
bitwise operations and write-back will write 0xFFFFFFFF (or 0xFFFFFFFF
masked in the disable function) to the INTEL_SPT_ACS_CTRL register,
setting all other writable bits.

For example, in pci_quirk_enable_intel_spt_pch_acs():

    pci_read_config_dword(dev, pos + PCI_ACS_CAP, &cap);
    pci_read_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, &ctrl);

    ctrl |= (cap & PCI_ACS_SV);
    ctrl |= (cap & PCI_ACS_RR);
    ctrl |= (cap & PCI_ACS_CR);
    ctrl |= (cap & PCI_ACS_UF);

    if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
        ctrl |= (cap & PCI_ACS_TB);

    pci_write_config_dword(dev, pos + INTEL_SPT_ACS_CTRL, ctrl);

Could these unchecked reads lead to hardware state corruption on transient
read failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6