Re: [PATCH v9 4/6] PCI: Refactor disable_acs_redir and config_acs param handling

[Wei Wang]

On 9/3/26 12:26 PM, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but user-provided ACS command line parameters silently wipe out standard IOMMU protections (SV, TB, UF) for unspecified bits by reverting to hardware defaults.


I plan to keep the patch as is unless someone sees a concrete case that warrants
a behavior change.


> --
>
> commit 62951886e4f26a51d147451d4aa7384b487c2ff6
> Author: Wei Wang <wei.w.wang@xxxxxxxxxxx>
>
> PCI: Refactor disable_acs_redir and config_acs param handling
>
> This commit splits the intertwined disable_acs_redir and config_acs parameter
> handling inside __pci_config_acs() into dedicated functions. It also improves
> parameter parsing by checking for a matching device before parsing flags and
> replacing complex string parsing with cleaner loops and switch statements.
>
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 587c013f04db3..6cffdbba99d34 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
>
> [ ... ]
>
> > +static void pci_param_disable_acs_redir(struct pci_dev *dev,
> > +					struct pci_acs *caps)
> > +{
> > +	const u16 acs_redir_mask = PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC;
> > +
> > +	if (!disable_acs_redir_param ||
> > +	    !pci_dev_match_disable_acs_redir(dev, disable_acs_redir_param) ||
> > +	    !pci_dev_specific_disable_acs_redir(dev))
> >   		return;
> >   
> > +	caps->ctrl = caps->fw_ctrl & ~acs_redir_mask;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does this overwrite silently
> discard the standard IOMMU protections (SV, TB, UF) established earlier?
>
> When an administrator uses pci=disable_acs_redir=..., pci_enable_acs()
> first enables OS-level protections via pci_std_enable_acs(dev, &caps).
>
> However, because this assignment bases the new control register completely on
> caps->fw_ctrl, any previously configured security bits are erased. This breaks
> PCIe isolation guarantees for the affected devices.

This mainly addresses how to handle the "x" bit.
kernel-parameters.txt mentions:
"
pci=config_acs=10x@pci:0:0 would configure all devices that support
ACS to enable P2P Request Redirect, disable Translation Blocking,
and leave Source Validation unchanged from whatever power-up
or firmware set it to.
"

So the original intent was to leave the state as the firmware left it.

Regarding IOMMU grouping, the impact should be minimal since groups
are made later from the register as actually written (via
pci_acs_enabled()), so there is no false claim of isolation.


> [ ... ]
>
> > +static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
> > +{
>
> [ ... ]
>
> > +	invalid_bits = (enabled_bits | disabled_bits) & ~valid_ctrl;
> > +	if (invalid_bits) {
> > +		pci_dbg(dev, "Ignoring unsupported ACS bits: %#06x\n",
> > +			invalid_bits);
> > +		enabled_bits &= valid_ctrl;
> > +		disabled_bits &= valid_ctrl;
> > +	}
> >   
> > -	pci_dbg(dev, "ACS mask  = %#06x\n", mask);
> > -	pci_dbg(dev, "ACS flags = %#06x\n", flags);
> > -	pci_dbg(dev, "ACS control = %#06x\n", caps->ctrl);
> > -	pci_dbg(dev, "ACS fw_ctrl = %#06x\n", caps->fw_ctrl);
> > +	pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n",
> > +		enabled_bits, disabled_bits);
> >   
> > -	/*
> > -	 * For mask bits that are 0, copy them from the firmware setting
> > -	 * and apply flags for all the mask bits that are 1.
> > -	 */
> > -	caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);
> > +	caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
>
> [Severity: High]
> This is a pre-existing issue, but does this assignment also inadvertently wipe
> out the IOMMU protections set up by pci_std_enable_acs()?
>
> Similar to pci_param_disable_acs_redir(), when pci=config_acs=... is used,
> all un-masked bits revert to their fw_ctrl hardware default state rather than
> persisting their pci_std_enable_acs() modifications.
>
> This could potentially allow DMA spoofing or unauthorized memory access on
> devices where the admin only intended to tweak specific ACS bits.

Similar to the above, RR/CR are already cleared by the admin's request, so
subsequent device grouping will work as expected.