Re: [PATCH] PCI: qcom: Honor IOMMU provider's #iommu-cells in qcom_pcie_config_sid_1_9_0()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Out-of-bounds memory read when `iommu-map` property length is less than 2 cells.
- [High] Unvalidated `#iommu-cells` property causes integer overflow, leading to out-of-bounds memory reads or divide-by-zero undefined behavior.

Pre-existing issues:
- [High] Infinite loop if the number of `iommu-map` entries exceeds the fixed BDF-to-SID hardware table capacity.
--

commit 5e6355134714e5a9f21664a5ac644c1347a1924b
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI: qcom: Honor IOMMU provider's #iommu-cells in qcom_pcie_config_sid_1_9_0()

This commit updates qcom_pcie_config_sid_1_9_0() to properly handle
iommu-map entries where the IOMMU provider uses more than one iommu-cell.
It dynamically calculates the per-entry stride based on the IOMMU's
#iommu-cells property instead of assuming a fixed four-word stride.

> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index b58a607b713f5..42cbeef083caf 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
[ ... ]
> @@ -1143,50 +1143,90 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
>  
>  static int qcom_pcie_config_sid_1_9_0(struct qcom_pcie *pcie)
>  {
[ ... ]
> +	if (size % sizeof(*map)) {
> +		dev_err(dev, "Malformed iommu-map property\n");
> +		return -EINVAL;
> +	}
> +	nr_cells = size / sizeof(*map);
> +
> +	/*
> +	 * Each iommu-map entry is: rid-base (1 cell), phandle (1 cell),
> +	 * IOMMU specifier (#iommu-cells cells), length (1 cell). Read
> +	 * #iommu-cells from the IOMMU provider referenced by the first
> +	 * entry to compute the per-entry stride.
> +	 */
> +	phandle = be32_to_cpu(map[1]);

[Severity: High]
Can this cause an out-of-bounds read if the iommu-map property is too short?

The function verifies the property size is a multiple of 4 bytes, but fails
to verify the minimum required length (at least 2 cells) before accessing
map[1] to extract the phandle. A malformed device tree with an iommu-map
property of 4 bytes (1 cell) could trigger an out-of-bounds memory read
during probe.

> +	iommu_np = of_find_node_by_phandle(phandle);
> +	if (!iommu_np) {
> +		dev_err(dev, "Failed to find IOMMU node in iommu-map\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_property_read_u32(iommu_np, "#iommu-cells", &iommu_cells))
> +		iommu_cells = 1;
> +	of_node_put(iommu_np);
> +
> +	entry_cells = 3 + iommu_cells;

[Severity: High]
Does this code allow an unvalidated #iommu-cells property to cause an
integer overflow?

If a device tree specifies an excessively large value for #iommu-cells,
such as 0xFFFFFFFE, entry_cells = 3 + iommu_cells will wrap around to 1.
If the value is 0xFFFFFFFD, it wraps to 0.

> +
> +	/*
> +	 * Retain backward compatibility with DTs that describe iommu-map
> +	 * with 4-cell entries against an IOMMU declaring #iommu-cells = 2,
> +	 * matching the fallback in drivers/of/base.c::of_check_bad_map().
> +	 */
> +	if (iommu_cells == 2 && !(nr_cells % 4)) {
[ ... ]
> +	}
> +
> +	if (nr_cells % entry_cells) {

[Severity: High]
Could the above integer overflow lead to a divide-by-zero here?

If entry_cells wrapped to 0 due to an overflow, this modulo operation will
trigger undefined behavior (divide-by-zero). If it wrapped to a small
non-zero number like 1, it could pass this check and result in
nr_map = nr_cells / 1, potentially causing out-of-bounds reads in the
subsequent loop.

> +		dev_err(dev, "Malformed iommu-map property\n");
> +		return -EINVAL;
> +	}
> +	nr_map = nr_cells / entry_cells;
> +
>  	/* Enable BDF to SID translation by disabling bypass mode (default) */
>  	val = readl(pcie->parf + PARF_BDF_TO_SID_CFG);
[ ... ]
>  	/* Look for an available entry to hold the mapping */
>  	for (i = 0; i < nr_map; i++) {
> -		__be16 bdf_be = cpu_to_be16(map[i].bdf);
> -		u32 val;
> +		u32 bdf = be32_to_cpu(map[i * entry_cells]);
> +		u32 sid = be32_to_cpu(map[i * entry_cells + 2]);
> +		__be16 bdf_be = cpu_to_be16(bdf);
>  		u8 hash;
>  
>  		hash = crc8(qcom_pcie_crc8_table, (u8 *)&bdf_be, sizeof(bdf_be), 0);

[Severity: High]
This is a pre-existing issue, but can this loop cause a kernel hard lockup?

Just below this in qcom_pcie_config_sid_1_9_0(), there is a linear probing
loop to handle hash collisions:

		while (val) {
			u8 current_hash = hash++;
			...
			val = readl(bdf_to_sid_base + hash * sizeof(u32));
		}

The hardware table has a capacity of 256 slots (CRC8_TABLE_SIZE). If a device
tree specifies more than 256 entries in iommu-map (or if hash collisions fill
all slots), the while (val) loop continuously increments the 8-bit hash index
and reads occupied slots, wrapping indefinitely. It lacks a termination
condition for a full translation table.

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