Re: [PATCH v10 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:
- [Medium] pci: missing port-type validation allows setting reserved UMAC/URRC bits on Root Ports
- [Medium] pci: unconditional check for reserved DMAC/UMAC encodings breaks config_acs on non-ECAP devices
--

commit d128af062fd915f74bfeafe2fdd8a9e6bffb5ab9
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 ffb489e4cbef6..4942bb6f1e111 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -982,6 +985,10 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
>  	u16 shift = 0, enabled_bits = 0, disabled_bits = 0, invalid_bits;
>  	u16 valid_ctrl = dev->acs_capabilities & PCI_ACS_BASIC_CTRL_MASK;
>  	const char *p, *seg;
> +	u16 new_ctrl;
> +
> +	if (dev->acs_capabilities & PCI_ACS_ECAP)
> +		valid_ctrl |= PCI_ACS_ECAP_CTRL_MASK;

[Severity: Medium]
Does this missing port-type validation allow setting reserved UMAC/URRC
bits on Root Ports?

Because PCI_ACS_ECAP_CTRL_MASK unconditionally includes the UMAC and URRC
bits, an administrator could use the config_acs= boot parameter to
overwrite these reserved bits on a Root Port device that supports
PCI_ACS_ECAP. 

The PCIe Base Specification states these bits must be hardwired to 0 for Root
Ports, so permitting them to be configured here could lead to undefined
behavior on poorly-behaved hardware.

>  
>  	if (!config_acs_param || !valid_ctrl)
>  		return;
[ ... ]
> @@ -1023,10 +1030,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) {

[Severity: Medium]
Does this unconditional check for reserved DMAC/UMAC encodings break
config_acs on non-ECAP devices?

If a non-ECAP device has firmware or hardware quirks that leave bits 8-11
initialized to 1s, this check will falsely trigger. Because it runs
unconditionally, it applies ECAP-specific validation constraints to non-ECAP
devices where the bits are just reserved. 

This will cause the function to abort, preventing the user from configuring
even basic, supported ACS controls on the device.

> +		pci_err(dev, "ACS DMAC/UMAC would be set to reserved value\n");
> +		return;
> +	}
> +
>  	pci_dbg(dev, "ACS enabled: %#06x, disabled: %#06x\n",
>  		enabled_bits, disabled_bits);
>  
> -	caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
> +	caps->ctrl = new_ctrl;
>  }
>  
>  /**

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