Re: [PATCH v7 1/6] PCI: Validate ACS enable flags against device-specific ACS capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The new validation logic unconditionally fails when parsing the `disable_acs_redir` parameter, completely breaking the ability to disable ACS redirection.
- [High] The validation logic incorrectly treats the 'Egress Control Vector Size' field as capability flags, breaking the configuration of Enhanced ACS features and allowing reserved bit corruption.
- [Medium] The complete removal of the `mask` bounds validation allows malformed user strings to clear reserved bits in the ACS Control Register.
--

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

PCI: Validate ACS enable flags against device-specific ACS capabilities

The ACS flag validation is updated to use dev->acs_capabilities instead
of the full set of ACS control bits, ensuring only supported ACS features
are enabled. Note that a regression regarding the disable_acs_redir
parameter was fixed later in the patch series by commit 2b4c7af1c4a60
(PCI: Refactor disable_acs_redir and config_acs param handling).

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 783e71962cc61..e017200694d8a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -931,12 +931,6 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  			}
>  		}
>  
> -		if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
> -			    PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
> -			pci_err(dev, "Invalid ACS flags specified\n");
> -			return;
> -		}
> -

Does the removal of the mask validation allow malformed user strings to
clear reserved bits in the ACS Control Register?

If a user specifies 0 in the command line for an upper bit position, that
bit is set in mask but remains clear in flags. Because flags is 0 at that
position, the new validation checks will trivially pass.

The function later evaluates:

caps->ctrl = (caps->fw_ctrl & ~mask) | (flags & mask);

which would unconditionally clear the corresponding bits in the device's ACS
Control register, potentially violating the PCIe specification that dictates
reserved bits must be preserved or read-only.

>  		ret = pci_dev_str_match(dev, p, &p);
>  		if (ret < 0) {
[ ... ]
> @@ -959,6 +953,12 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  	if (!pci_dev_specific_disable_acs_redir(dev))
>  		return;
>  
> +	if (flags & ~dev->acs_capabilities) {
> +		pci_err(dev, "Invalid ACS enable flags specified: %#06x\n",
> +			(u16)(flags & ~dev->acs_capabilities));
> +		return;
> +	}
> +

Will this validation unconditionally fail when parsing the disable_acs_redir
parameter, breaking the ability to disable ACS redirection?

When the pci=disable_acs_redir parameter is used, __pci_config_acs() is
called with an inverted mask for flags, making acs_flags evaluate to 0xFFD3
as a 16-bit unsigned integer.

Since flags has its upper bits set, and ~dev->acs_capabilities also has its
upper bits set, the bitwise AND will result in a non-zero value. This seems
like it will instantly log an invalid flags error and return early, silently
failing to disable ACS redirection.

Could treating the Egress Control Vector Size field as capability flags break
the configuration of Enhanced ACS features?

dev->acs_capabilities is populated by reading the 16-bit PCI_ACS_CAP register,
where bits 8-15 contain the Egress Control Vector Size integer. Validating
against the full 16-bit value incorrectly uses the vector size as a bitmask
of valid capabilities.

If a user provides a parameter to enable Enhanced ACS controls, the validation
might erroneously reject them unless the device's vector size happens to
have those exact bits set. Conversely, if a device has a non-zero egress
vector size, a user could pass flags corresponding to the set bits in the
vector size, allowing unintended modification of reserved bits in
PCI_ACS_CTRL.

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