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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bounds checking in pci_param_config_acs() breaks backwards compatibility for padded parameters and contains an integer underflow that leads to Undefined Behavior.
- [Medium] Silenced logging for disable_acs_redir reduces configuration observability.
--

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

PCI: Refactor disable_acs_redir and config_acs param handling

This commit refactors the ACS parameter handling by splitting the old
__pci_config_acs function into dedicated functions for disable_acs_redir
and config_acs parameters.

> --- 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)
> +{
> +	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;
> +}

The previous implementation printed a pci_info message when ACS control was
modified. Is it intentional that pci_param_disable_acs_redir no longer logs
confirmation that the redirect was disabled?

This might make debugging device isolation issues more difficult without the
dmesg confirmation.

[ ... ]

> +static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
> +{
> +	u16 shift = 0, max_shift = fls(dev->acs_capabilities) - 1;

If dev->acs_capabilities is 0, fls(0) evaluates to 0. Will 0 - 1 underflow
the u16 max_shift variable to 65535, effectively bypassing the bounds check
below?

> +	u16 enabled_bits = 0, disabled_bits = 0;
> +	const char *p, *seg;

[ ... ]

> +	p = strchr(seg, '@');
> +	/* Parse bitstring backwards from '@' */
> +	while (p > seg) {
> +		if (shift > max_shift) {
> +			pci_err(dev, "ACS flag bit %d exceed range %d\n",
> +				shift, max_shift);
> +			return;
> +		}

The previous implementation gracefully allowed parameter padding that exceeded
the capability mask, only erroring if unsupported bits were actively enabled.

Does aborting here when shift exceeds max_shift break backwards compatibility
for existing working kernel boot parameters (like config_acs=00000000@...)
on devices with fewer capabilities?

> +
> +		switch (*--p) {
> +		case '1':
> +			enabled_bits |= BIT(shift);
> +			break;

If the bounds check is bypassed due to the max_shift underflow mentioned
above, could shift increment without bounds for long user strings?

If shift exceeds the width of an unsigned long, BIT(shift) could trigger
undefined behavior. Additionally, since enabled_bits is a u16, shift amounts
of 16 or greater will silently truncate.

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