Re: [PATCH v3] PCI: Reserve prefetchable window headroom for Resizable BARs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `resource_size_roundup_pow2` function invokes undefined behavior on 32-bit systems by shifting a 32-bit integer by 32 or more.
- [High] Concurrent execution of `pci_pack_low_reserve` during PCIe hotplug or asynchronous probing causes data races on the static budget array, leading to memory corruption.
- [Medium] The `pci_pack_low_reserve` function permanently leaks budget state and stores stale root bus pointers across device unbinds/hot-removals.
--

commit 3d18f0cac0d66de7c502abb13e0bec070d238ae4
Author: Geramy Loveless <gloveless@jqluv.com>

PCI: Reserve prefetchable window headroom for Resizable BARs

This commit reserves prefetchable window headroom during bridge sizing
for the maximum size of downstream Resizable BARs, allowing a driver
to grow a Resizable BAR later. It also demotes small prefetchable BARs
to the non-prefetchable window when sharing with a massive prefetchable BAR.

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index de9bbccda21fd..1094f618bb1ff 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
[ ... ]
> +/* Demote only if the host's 32-bit aperture has room (tracked per host). */
> +static bool pci_pack_low_reserve(struct pci_dev *dev, resource_size_t bytes)
> +{
> +	static struct { struct pci_bus *root; u64 used; } budget[32];
> +	static int nbudget;
> +	struct pci_bus *root = dev->bus;
> +	struct resource *r;
> +	u64 aperture = 0, *usedp = NULL;
> +	int i;
> +
[ ... ]
> +	for (i = 0; i < nbudget; i++) {
> +		if (budget[i].root == root) {

[Severity: Medium]
If a root bus is hot-removed, could its memory address be reallocated to a
new root bus, causing false matches here and inheriting the exhausted
budget from the stale pointer?

> +			usedp = &budget[i].used;
> +			break;
> +		}
> +	}
> +	if (!usedp) {
> +		if (nbudget >= (int)ARRAY_SIZE(budget))
> +			return false;
> +		budget[nbudget].root = root;
> +		budget[nbudget].used = 0;
> +		usedp = &budget[nbudget].used;
> +		nbudget++;

[Severity: High]
Could concurrent execution of pci_pack_low_reserve() during PCIe hotplug or
asynchronous probing cause data races here?

There are no synchronization primitives protecting the static budget array
and nbudget counter. If two concurrent threads attempt to insert a new root
bus when nbudget reaches the array size limit, could both bypass the bounds
check in a Time-of-Check to Time-of-Use (TOCTOU) race and cause an
out-of-bounds write, corrupting the kernel BSS section?

> +	}
> +	if (*usedp + bytes > aperture / 2)
> +		return false;
> +	*usedp += bytes;

[Severity: Medium]
Since there is no teardown or release hook to decrement the usage when a
device is removed, does this continuously accumulate the budget usage?

If a device is repeatedly hot-unplugged and re-plugged, or if a software bus
rescan is triggered via sysfs, does this permanently leak the tracked budget
until a reboot?

> +	return true;
> +}
[ ... ]
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index e8c94aa1d3c12..d8b8cb35b8bb6 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
[ ... ]
> +/* roundup_pow_of_two() is unsigned long and truncates 64-bit sizes on 32-bit. */
> +static resource_size_t resource_size_roundup_pow2(resource_size_t n)
> +{
> +	if (n && (n & (n - 1)))
> +		n = (resource_size_t)1 << fls64(n);

[Severity: High]
Does this shift operation in resource_size_roundup_pow2() invoke undefined
behavior on 32-bit systems?

If the requested bridge window size n is greater than 2GB (e.g. a 2GB BAR
plus an additional smaller BAR) on a system where resource_size_t is 32-bit,
fls64(n) will return 32. Since 1 is cast to a 32-bit type (resource_size_t),
shifting it by 32 or more results in undefined behavior in C.

> +	return n;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/10fb16fb-2069-4d89-b473-1006ae5abfbc@jqluv.com?part=1