Re: [PATCH 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: infinite loop in pci_find_free_ext_cap_offset() without TTL
- [High] pci: hardcoded 64-byte size in pci_find_free_ext_cap_offset() clobbers large capabilities
- [High] pci: out-of-bounds config access via integer underflow in pci_find_free_ext_cap_offset()
- [Medium] pci: stateless allocation in pci_find_free_ext_cap_offset() causes resource collisions
--

commit cd8d51b3f77c9157b1c6d30b169fe7c04efaecb1
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. It scans the extended capability chain and returns an
offset to a gap of at least the requested size, intending to allow VFIO
variant drivers to use unused space.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..31f70e2982b63 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 pos = PCI_CFG_SPACE_SIZE;
> +	int last_cap_start = 0;
> +	int free_offset;
> +	u32 header;
> +	int next;
> +
> +	if (!pci_is_pcie(dev))
> +		return 0;
> +
> +	while (pos >= PCI_CFG_SPACE_SIZE) {
> +		if (pci_read_config_dword(dev, pos, &header))
> +			break;
> +
> +		if (header == 0 || header == 0xffffffff)
> +			break;
> +
> +		last_cap_start = pos;
> +		next = PCI_EXT_CAP_NEXT(header);
> +		if (!next)
> +			break;
> +		pos = next;
> +	}

[Severity: High]
Could this loop indefinitely if a malicious or buggy PCI device presents an
extended capability chain where the next pointer points backwards or forms
a cycle? It looks like this traversal lacks the Time-To-Live (TTL) limit
that is present in other helpers like pci_find_next_ext_capability().

> +
> +	/*
> +	 * Use space after the last capability. We align to the next capability
> +	 * boundary (typically 4-byte aligned). Start at a safe offset past the
> +	 * last capability - capabilities vary in size, so use a conservative
> +	 * offset. Most extended capabilities are <= 64 bytes.
> +	 */
> +	if (last_cap_start) {
> +		free_offset = last_cap_start + 64;
> +		free_offset = ALIGN(free_offset, 4);

[Severity: High]
Is a hardcoded 64 bytes always sufficient here? Some standard capabilities
are larger than 64 bytes (for example, AER is 72 bytes, and VSEC/DVSEC can
be up to 4KB). If the device's last extended capability exceeds 64 bytes,
would this offset fall inside the bounds of the active capability and lead
to data corruption when written?

> +		if (PCI_CFG_SPACE_EXP_SIZE - free_offset >= size)

[Severity: High]
Can this subtraction result in an integer underflow?

If the last capability starts near the end of the configuration space such
that free_offset exceeds PCI_CFG_SPACE_EXP_SIZE (4096), the subtraction
would yield a negative integer. Because size is a size_t, wouldn't the
negative integer be implicitly promoted to a very large unsigned value,
bypassing this capacity check and returning an out-of-bounds offset?

> +			return free_offset;

[Severity: Medium]
Because this offset is returned without reserving the space in the
capability chain, could multiple callers receive the exact same offset?
This stateless allocation appears like it might cause multiple subsystems
or quirks to overwrite each other's data at the same offset.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_find_free_ext_cap_offset);

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