Re: [PATCH v2 1/7] PCI: Add pci_find_free_ext_cap_offset() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pci: zero-scan identifies zeroed payload of highest capability as free space
- [High] pci: out-of-bounds config space access due to hardcoded PCI_CFG_SPACE_EXP_SIZE
- [High] pci: config space corruption due to zero-scanning capability payloads
- [Medium] pci: function fails to identify completely empty extended config space
--

commit c1630045212cdfa197e241362843099e2b8fa29f
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

PCI: Add pci_find_free_ext_cap_offset() helper

This commit adds a helper function to find free space in the PCI extended
configuration space for VFIO variant drivers to use. It walks the extended
capability chain and scans for contiguous zeroed space.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8cc6a89130b53..ae234c9a2a29b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> +int pci_find_free_ext_cap_offset(struct pci_dev *dev, size_t size)
> +{
> +	int ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> +	int pos = PCI_CFG_SPACE_SIZE;
> +	int start = 0, run = 0;
> +	int highest_pos = 0;
> +	u32 header;
> +	int offset;
> +
> +	if (!pci_is_pcie(dev) || dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> +		return 0;
> +
> +	if (size == 0 || size > PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE)
> +		return 0;
> +
> +	if (pci_read_config_dword(dev, pos, &header))
> +		return 0;
> +	if (header == 0 || header == 0xffffffff)
> +		return 0;

[Severity: Medium]
Will this early exit prevent the function from utilizing completely empty
extended configuration space?

If the header at PCI_CFG_SPACE_SIZE is 0 or 0xffffffff, it indicates
the device has no extended capabilities whatsoever. In this case, the entire
extended configuration space up to dev->cfg_size is unused and free.

Returning 0 here prevents drivers from utilizing this completely empty
configuration space.

> +
> +	while (ttl-- > 0) {
[ ... ]
> +	if (!highest_pos)
> +		return 0;
> +
> +	for (offset = highest_pos; offset <= (int)PCI_CFG_SPACE_EXP_SIZE - 4;
> +	     offset += 4) {

[Severity: High]
Does this loop bound cause out-of-bounds configuration space accesses?

The loop condition is hardcoded to PCI_CFG_SPACE_EXP_SIZE - 4 (4092 bytes),
completely ignoring dev->cfg_size. Many PCIe devices and host bridges have
restricted configuration sizes (e.g., 512, 1024, or 2048 bytes) due to hardware
limits or quirks.

For these devices, calling pci_read_config_dword() outside their physical
configuration space can trigger unsupported request errors, leading to machine
check exceptions or system errors.

If the bridge returns 0 for these out-of-bounds reads, the function could
falsely identify non-existent space as free, leading to out-of-bounds writes.

[Severity: High]
Could starting the scan exactly at highest_pos cause the function to scan
an active capability's payload?

The scan begins exactly at the header of the highest-offset capability
(highest_pos), instead of skipping past its actual size.

> +		u32 val;
> +
> +		if (pci_read_config_dword(dev, offset, &val))
> +			break;
> +
> +		if (val == 0) {
> +			if (run == 0)
> +				start = offset;
> +			run += 4;
> +			if (run >= (int)size)
> +				return start;
> +		} else {

[Severity: High]
Will this zero-scanning approach corrupt active capabilities?

Because the scan doesn't skip the actual size of the highest capability, it
reads the payload registers (such as status, mask, or log registers) which
often naturally default to zero.

If the scanner finds a contiguous block of zeroes inside the active payload, it
will falsely return this internal offset as 'free space'.

Callers writing to this offset will then silently overwrite and corrupt the
active capability's state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011074514.515894-1-jtornosm@redhat.com?part=1