Re: [PATCH AUTOSEL 6.18-5.10] ata: ahci: fail probe if BAR too small for claimed ports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The BAR size validation incorrectly assumes the maximum port index is bounded by HOST_CAP.NP, but the driver actually accesses ports up to fls(HOST_PORTS_IMPL), allowing out-of-bounds MMIO accesses to bypass the check.
- [High] The BAR size validation relies strictly on HOST_CAP.NP and rejects the device if the BAR is smaller than required for the maximum claimable ports, which can break working devices.
--

commit 93586245aa8cac6edeb231c78f24de78d4e9a254
Author: liyouhong <liyouhong@kylinos.cn>
Subject: ata: ahci: fail probe if BAR too small for claimed ports

This patch adds validation in ahci_init_one() to check if the BAR size is
sufficient for the number of ports claimed in HOST_CAP.NP. If the required
size exceeds the actual BAR size, the probe fails with -ENODEV to prevent
kernel panics from out-of-bounds MMIO access.

Link: https://lore.kernel.org/all/20260422080322.1006592-1-dayou5941@163.com/

> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 7a7f88b3fa2b1..bbf6e83abd880 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1874,6 +1874,24 @@ static ssize_t remapped_nvme_show(struct device *dev,
>  
>  static DEVICE_ATTR_RO(remapped_nvme);
>  
> +static int ahci_validate_bar_size(struct pci_dev *pdev, int bar,
> +				  struct ahci_host_priv *hpriv)
> +{
> +	u32 cap = readl(hpriv->mmio + HOST_CAP);
> +	unsigned int max_ports = ahci_nr_ports(cap);
> +	u32 last_port_end = 0x100 + (max_ports * 0x80);

[Severity: High]
Does this validation actually prevent all out-of-bounds MMIO accesses?

The validation uses HOST_CAP.NP (via ahci_nr_ports) to determine the required
BAR size, ignoring the actually implemented ports in HOST_PORTS_IMPL.

Looking at ahci_save_initial_config(), it only clears port_map if the total
count of set bits (map_ports) exceeds CAP.NP:

drivers/ata/libahci.c:ahci_save_initial_config() {
	...
		if (map_ports > ahci_nr_ports(cap)) {
			dev_warn(dev,
				 "implemented port map (0x%lx) contains more ports than nr_ports (%u), using nr_ports\n",
				 port_map, ahci_nr_ports(cap));
			port_map = 0;
		}
	...
}

If a controller reports CAP.NP = 0 (1 port) but sets bit 31 in HOST_PORTS_IMPL,
map_ports is 1. Since 1 > 1 is false, port_map is not cleared.

The loop will then execute for index 31, accessing MMIO out of bounds:

drivers/ata/libahci.c:ahci_save_initial_config() {
	...
	for_each_set_bit(i, &port_map, AHCI_MAX_PORTS) {
		if (hpriv->saved_port_cap[i])
			continue;

		port_mmio = __ahci_port_base(hpriv, i);
	...
}

If the BAR size is 0x200, ahci_validate_bar_size() allows the probe to
continue (since max_ports is 1, and 0x180 < 0x200), resulting in an OOB access
and the exact panic this patch intended to prevent.

> +	resource_size_t bar_size = pci_resource_len(pdev, bar);
> +
> +	if (last_port_end > bar_size) {
> +		dev_warn(&pdev->dev,
> +			 "BAR%d too small for %u ports (last port ends at %#x, BAR %pa)\n",
> +			 bar, max_ports, last_port_end, &bar_size);
> +		return -ENODEV;

[Severity: High]
Could this strict validation break working devices?

If a device's generic IP block hardcodes CAP.NP to 31 (claiming 32 ports,
requiring a 0x1100 byte BAR), but the manufacturer only implemented 2 ports
(reflected in HOST_PORTS_IMPL) and mapped a standard 4KB (0x1000 byte) BAR,
the validation will fail (0x1100 > 0x1000).

Before this patch, the driver correctly relied on HOST_PORTS_IMPL to determine
which ports to access and safely initialized within the 4KB BAR. This patch
introduces a strict check that rejects the device if the BAR is smaller than
required for the maximum claimable ports in CAP.NP.

Should this warn instead of failing the probe entirely, or base the check on
the maximum port actually enabled in HOST_PORTS_IMPL?

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-166-sashal@kernel.org?part=1