Re: [PATCH v7 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 1 potential issue(s) to consider:
- [High] The `config_acs=` boot parameter parser incorrectly validates the newly added ACS Enhanced Capability controls (bits 8-12), unconditionally rejecting valid configurations and bypassing administrator-defined isolation policies.
--

commit 7640f1b4b51e699ac56a5798f5e8dbb9114e4b25
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/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0c159ab59e14b..32e82859089c8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5265,13 +5265,22 @@ Kernel parameters
>  				flags.
>  
>  				ACS Flags is defined as follows:
> -				  bit-0 : ACS Source Validation
> -				  bit-1 : ACS Translation Blocking
> -				  bit-2 : ACS P2P Request Redirect
> -				  bit-3 : ACS P2P Completion Redirect
> -				  bit-4 : ACS Upstream Forwarding
> -				  bit-5 : ACS P2P Egress Control
> -				  bit-6 : ACS Direct Translated P2P
> +				  bit-0     : ACS Source Validation
> +				  bit-1     : ACS Translation Blocking
> +				  bit-2     : ACS P2P Request Redirect
> +				  bit-3     : ACS P2P Completion Redirect
> +				  bit-4     : ACS Upstream Forwarding
> +				  bit-5     : ACS P2P Egress Control
> +				  bit-6     : ACS Direct Translated P2P
> +				  bit-7     : ACS I/O Request Blocking
> +				  bit-9:8   : ACS DSP Memory Target Access Ctrl
> +				      00    : Direct Request access enabled
> +				      01    : Request blocking enabled
> +				      10    : Request redirect enabled
> +				      11    : Reserved
> +				  bit-11:10 : ACS USP Memory Target Access Ctrl
> +				              Same encoding as bit-9:8
> +				  bit-12    : ACS Unclaimed Request Redirect Ctrl
>  				Each bit can be marked as:
>  				  '0' – force disabled
>  				  '1' – force enabled

Does this configuration introduce a regression where valid inputs for bits 8
through 12 are unconditionally rejected?

Looking at the parser in drivers/pci/pci.c:pci_param_config_acs(), it validates
the requested bits against the hardware capabilities:

	u16 shift = 0, max_shift = fls(dev->acs_capabilities) - 1;
	...
		if (shift > max_shift) {
			pci_err(dev, "ACS flag bit %d exceed range %d\n",
				shift, max_shift);
			return;
		}
	...
	if (enabled_bits & ~dev->acs_capabilities) {
		pci_err(dev, "Invalid ACS enable flags specified: %#06x\n",
			(u16)(enabled_bits & ~dev->acs_capabilities));
		return;
	}

Since bits 8 through 15 of the ACS Capability Register represent the Egress
Control Vector Size rather than capability flags, validating bits 8-12 against
dev->acs_capabilities appears to be incorrect.

If the Egress Control Vector Size is 0, max_shift will be 7, and the parser
will reject any attempt to configure bits 8 through 12. If it is non-zero, the
bitwise validation will fail unpredictably depending on the size value.

Will this cause the requested administrator-defined isolation policies to be
bypassed?

[ ... ]

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