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

On Thu, Aug 06, 2026 at 11:40:55AM +0000, [email protected] wrote:
> 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?

A failed read is always followed by a failed write, so the register is not
changed. pci_generic_config_read() and pci_generic_config_write() both use
bus->ops->map_bus(), and on x86 pci_conf1_read/write and pci_mmcfg_read/write
have the same checks. If the read fails, the write to the same offset fails
too, so INTEL_SPT_ACS_CTRL never gets 0xFFFFFFFF.

The check function is different. It returns an isolation answer. On a failed
read, the old code passed ~0 to pci_acs_ctrl_enabled(), where
(req & 0xffff) == req is always true, so it reported full isolation from
garbage data. The enable and disable helpers return no such answer, and they
already write nothing when the read fails.

It is false alarm.

Thanks

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