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

On 5/7/26 5:07 AM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

Yes. I think it's better to have the pci_info message printed in
pci_enable_acs():

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b684ae4e92b5..dcc591d679d5 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1008,9 +1008,6 @@ static void pci_param_config_acs(struct pci_dev *dev, struct pci_acs *caps)
                 enabled_bits, disabled_bits);
  
         caps->ctrl = (caps->fw_ctrl | enabled_bits) & ~disabled_bits;
-
-       pci_info(dev, "Configured ACS to %#06x (FW default: %#06x)\n",
-                caps->ctrl, caps->fw_ctrl);
  }
  
  /**
@@ -1080,6 +1077,9 @@ void pci_enable_acs(struct pci_dev *dev)
         pci_param_disable_acs_redir(dev, &caps);
         pci_param_config_acs(dev, &caps);
  
+       pci_info(dev, "Configured ACS to %#06x (FW default: %#06x)\n",
+                caps->ctrl, caps->fw_ctrl);
+
         pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);


> 
> [ ... ]
> 
>> +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?

pci_enable_acs() has already gated the execution via checking dev->acs_cap,
so if the acs_cap is 0, it should have returned earlier already.

It seems not possible that a device has an acs_cap but dev->acs_capabilities is
0, which means the device added acs cap but the cap is empty (no acs features).


> 
>> +	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?

Yes, use "break" here is better than "return". We should allow the valid
configs (< max_shift) applying to device. Zeroing those exceed the max_shift
isn't necessary, as the device does not support them.

> 
>> +
>> +		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.

As mentioned above, max_shift wouldn't underflow, so it won't be an issue
here then.