Re: [PATCH 3/5] PCI: Place resources to either edge of the window

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: undefined behavior from 64-bit resource sizes on 32-bit systems
- [Medium] pci: res->end + 1 overflow breaks natural alignment logic
--

commit 65c3dea851ed27dd4bf3576e0a9387eaa7359798
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Subject: PCI: Place resources to either edge of the window

This commit updates the PCI resource assignment phase to use a window edge
aware resource assignment algorithm. It alters the greedy assignment logic
to place resources either to the left or right edge of the window to avoid
fragmenting contiguous free space.

> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index eacce9e2486bf..1ab5d167ab5c6 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
[ ... ]
> @@ -262,16 +265,80 @@ resource_size_t pci_resource_alignment(const struct pci_dev *dev,
>  	return resource_alignment(res);
>  }
>  
> +static resource_size_t pci_max_natural_size(const struct resource *res,
> +					    resource_size_t *max_align)
> +{
> +	resource_size_t size = resource_size(res);
> +	resource_size_t powof2, natural_start;
> +
> +	*max_align = 1;
> +	if (!size)
> +		return 0;
> +
> +	powof2 = rounddown_pow_of_two(size);

[Severity: High]
Does this code inadvertently truncate 64-bit resource sizes on 32-bit systems?

When a 32-bit kernel is built with a 64-bit phys_addr_t (like x86 PAE or
ARM LPAE), passing a 64-bit size into rounddown_pow_of_two() casts it to an
unsigned long. If the size is a multiple of 4GB (which is common for large
GPU BARs), the lower 32 bits are silently truncated to 0, causing powof2 to
be evaluated as 0.

> +	natural_start = ALIGN(res->start, powof2);
> +	if (natural_start >= ALIGN_DOWN(res->end + 1, powof2)) {

[Severity: Medium]
Could this boundary check overflow if the window ends exactly at ~0U?

If a 32-bit system maps PCI space just below the 4GB boundary, res->end
could be 0xFFFFFFFF. In that scenario, res->end + 1 wraps to 0, which
causes ALIGN_DOWN(0, powof2) to return 0. The check natural_start >= 0 then
unconditionally evaluates to true for unsigned types, incorrectly forcing
the algorithm to halve powof2 and potentially failing to pack memory windows
tightly.

> +		powof2 = max(powof2 / 2, 1U);
> +		natural_start = ALIGN(res->start, powof2);
> +	}
> +
> +	if (natural_start) {
> +		*max_align <<= __ffs(natural_start);

[Severity: High]
Does this trigger undefined behavior when passing a truncated 64-bit value?

Similar to rounddown_pow_of_two(), __ffs() takes an unsigned long. If a
64-bit natural_start is a multiple of 4GB, the cast truncates it to 0.
Passing 0 to __ffs() invokes undefined behavior, which can lead to
unpredictable results or kernel panics during boot (such as garbage shift
amounts or arch-specific trap instructions on zero inputs).

> +	} else {
> +		/*
> +		 * Zero address has infinite alignment, return the largest
> +		 * representable number even if it's not a power of two.
> +		 */
> +		*max_align = RESOURCE_SIZE_MAX;
> +	}
> +
> +	return powof2;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923131757.7792-1-ilpo.jarvinen@linux.intel.com?part=3