Re: [PATCH v9 5/6] PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional validation of DMAC and UMAC fields rejects valid user ACS configurations on non-ECAP devices and fails to account for kernel-corrected states.
- [Medium] ACS Enhanced Capability access controls are unconditionally applied to unsupported PCIe port types, writing to reserved bits.
--

commit e1302a2bae5cac8f4c54de9a2a416f24d9b8a517
Author: Wei Wang <wei.w.wang@hotmail.com>

PCI: Enable the enhanced ACS controls introduced by PCI_ACS_ECAP

The ACS Enhanced Capability introduces several new access controls to
improve device isolation. These new controls are particularly important
for device passthrough in virtualization scenarios.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6cffdbba99d34..50a3b61f9898e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1026,10 +1029,23 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
>  		disabled_bits &= valid_ctrl;
>  	}
>  
> +	/*
> +	 * DMAC/UMAC are 2-bit fields where encoding 0b11 is reserved. The
> +	 * final value can land on 0b11 not only when the user enables both
> +	 * bits, but also when they enable one bit, e.g. "1x", while fw_ctrl
> +	 * already has the other set (e.g. "01").
> +	 */
> +	new_ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
> +	if (FIELD_GET(PCI_ACS_DMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD ||
> +	    FIELD_GET(PCI_ACS_UMAC_MASK, new_ctrl) == PCI_ACS_MAC_RSVD) {
> +		pci_err(dev, "ACS DMAC/UMAC would be set to reserved value\n");
> +		return;
> +	}

[Severity: High]
Does this unconditional check against reserved DMAC/UMAC values reject valid
user configurations on non-ECAP devices?

If a user boots with the config_acs parameter to configure a non-ECAP device,
but its firmware leaves bits 8:9 or 10:11 as 0b11, this FIELD_GET check detects
PCI_ACS_MAC_RSVD and aborts the configuration, preventing the user from
applying basic ACS settings.

Additionally, for ECAP devices with buggy firmware defaults (0b11), building
the new control register using caps->fw_ctrl instead of the kernel-corrected
caps->ctrl revives the invalid firmware state, instantly triggering this
validation failure and rejecting unrelated user flags.

[ ... ]

> @@ -1051,6 +1067,20 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	/* Upstream Forwarding */
>  	caps->ctrl |= (dev->acs_capabilities & PCI_ACS_UF);
>  
> +	/*
> +	 * Enable Unclaimed Request Redirect Control, I/O Request Blocking,
> +	 * and Downstream and Upstream Port Memory Target Access Redirect.
> +	 */
> +	if (dev->acs_capabilities & PCI_ACS_ECAP) {
> +		caps->ctrl |= PCI_ACS_IB;
> +		FIELD_MODIFY(PCI_ACS_DMAC_MASK, &caps->ctrl, PCI_ACS_MAC_RR);

[Severity: Medium]
Can this logic unconditionally apply these access controls to unsupported
port types?

If the kernel enumerates a PCIe Endpoint or Upstream Port that has the
PCI_ACS_ECAP bit set in its ACS capabilities, this unconditionally modifies
PCI_ACS_DMAC_MASK and sets PCI_ACS_IB.

Because ECAP controls like DMAC only apply to Root Ports and Downstream Ports,
applying them to Endpoints or Upstream Ports writes to reserved bits, which
could risk hardware misbehavior.

> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +			caps->ctrl |= PCI_ACS_URRC;
> +			FIELD_MODIFY(PCI_ACS_UMAC_MASK,
> +				     &caps->ctrl, PCI_ACS_MAC_RR);
> +		}
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB4393F337A792CB3258C2E073DCB62@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=5