Re: [PATCH] PCI: probe: Fix 64-bit limit truncation in prefetchable window

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If a 64-bit prefetchable memory window is disabled via its high registers (`mem_base_hi > mem_limit_hi`), the kernel fails to propagate this disabled state and may incorrectly enable the window using only the 32-bit low registers.
--

--- Patch [1]: [PATCH] PCI: probe: Fix 64-bit limit truncation in prefetchable window ---
commit 83ac911fac4a068a7685be844df59a43a0825bcf
Author: Mohamad Raizudeen <[email protected]>

PCI: probe: Fix 64-bit limit truncation in prefetchable window

This commit fixes a missing truncation check for 64-bit limit addresses on
32-bit platforms in pci_read_bridge_mmio_pref(). It ensures the memory
window is properly handled or disabled if the limit exceeds 32 bits, and
updates the error message to include both base and limit.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index dd0abbc63e18d..0cf2d1d351467 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -479,9 +479,10 @@ static void pci_read_bridge_mmio_pref(struct pci_dev *dev, struct resource *res,
>  	base = (pci_bus_addr_t) base64;
>  	limit = (pci_bus_addr_t) limit64;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
disabled 64-bit windows are handled just above this code.

If a 64-bit prefetchable memory window is disabled by firmware setting the
upper 32-bit registers such that mem_base_hi > mem_limit_hi, the kernel
skips appending the high bits to base64 and limit64:

        if (mem_base_hi <= mem_limit_hi) {
                base64 |= (u64) mem_base_hi << 32;
                limit64 |= (u64) mem_limit_hi << 32;
        }

If the lower 16-bit registers happen to evaluate to base <= limit (for
instance, if both base and limit are 0), will the subsequent check at the
end of pci_read_bridge_mmio_pref() incorrectly succeed?

        if (base <= limit) {
                region.start = base;
                region.end = limit + 0xfffff;
                pcibios_bus_to_resource(dev->bus, res, &region);

Could this cause the kernel to incorrectly enable a 1MB window at address 0
instead of marking the resource as disabled, potentially overlapping with
system RAM?

> -	if (base != base64) {
> -		pci_err(dev, "can't handle bridge window above 4GB (bus address %#010llx)\n",
> -			(unsigned long long) base64);
> +	if (base != base64 || limit != limit64) {
> +		pci_err(dev, "can't handle bridge window above 4GB (bus address %#010llx-%#010llx)\n",
> +			(unsigned long long) base64,
> +			(unsigned long long) limit64);
>  		return;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1