Re: [PATCH] PCI: host-generic: Fix NULL pointer dereference on 32-bit CAM systems

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete Fix / NULL Pointer Dereference in `pci_dw_ecam_bus_ops` on 32-bit platforms.
- [High] Missing NULL pointer validation in `pci_ecam_map_bus()` leading to invalid memory access.
- [High] Logic error in `pci_ecam_map_bus()` causes cross-device config space corruption for CAM mode.
--

--- Patch [1]: [PATCH] PCI: host-generic: Fix NULL pointer dereference on 32-bit CAM systems ---
commit 208f10f6fcd6715f0a5bbab0d769a909c797f297
Author: Steffen Persvold <spersvold@gmail.com>

PCI: host-generic: Fix NULL pointer dereference on 32-bit CAM systems

This commit fixes a NULL pointer dereference on 32-bit systems using CAM
by defining pci_generic_cam_ops with .add_bus and .remove_bus callbacks
and updating the host generic driver to reference it.

> diff --git a/drivers/pci/controller/pci-host-generic.c b/drivers/pci/controller/pci-host-generic.c
> index c1bc0d34348f4..9e85c6e9b4254 100644
> --- a/drivers/pci/controller/pci-host-generic.c
> +++ b/drivers/pci/controller/pci-host-generic.c

[ ... ]

> @@ -60,7 +51,7 @@ static const struct pci_ecam_ops pci_dw_ecam_bus_ops = {
>  

[Severity: High]
This is a pre-existing issue, but pci_dw_ecam_bus_ops shown just above this
context seems to be missing the same .add_bus and .remove_bus callbacks.

If a 32-bit system uses snps,dw-pcie-ecam or similar compatibles, does it
also crash with a NULL pointer dereference during bus enumeration because
the per-bus mapping is never populated?

>  static const struct of_device_id gen_pci_of_match[] = {
>  	{ .compatible = "pci-host-cam-generic",
> -	  .data = &gen_pci_cfg_cam_bus_ops },
> +	  .data = &pci_generic_cam_ops },
>  

[ ... ]

> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
> index 119de32ff07bd..a9b3bce2492f0 100644
> --- a/drivers/pci/ecam.c
> +++ b/drivers/pci/ecam.c
> @@ -208,6 +208,19 @@ const struct pci_ecam_ops pci_generic_ecam_ops = {
>  };
>  EXPORT_SYMBOL_GPL(pci_generic_ecam_ops);
>  
> +/* CAM ops */
> +const struct pci_ecam_ops pci_generic_cam_ops = {
> +	.bus_shift	= 16,
> +	.pci_ops	= {
> +		.add_bus	= pci_ecam_add_bus,
> +		.remove_bus	= pci_ecam_remove_bus,
> +		.map_bus	= pci_ecam_map_bus,

[Severity: High]
This isn't a bug introduced by this patch, but there might be a missing
pointer check in pci_ecam_map_bus() when pci_ecam_add_bus() fails on
32-bit systems.

If pci_ecam_add_bus() fails, for example due to an ioremap failure, the
per-bus pointer remains NULL. pci_ecam_map_bus() handles it like this:

drivers/pci/ecam.c:pci_ecam_map_bus() {
    ...
    if (per_bus_mapping) {
        base = cfg->winp[busn];
        ...
        return base + (bus_offset | devfn_offset | where);
    }
    ...
}

Since the offset is usually non-zero, it returns a small non-NULL invalid
pointer. Can this bypass the null check in callers like
pci_generic_config_read() and cause an invalid memory access?

[Severity: High]
This is also a pre-existing issue, but using pci_ecam_map_bus() for CAM ops
might cause cross-device config space corruption.

In pci_ecam_map_bus(), the register offset is unconditionally masked:

drivers/pci/ecam.c:pci_ecam_map_bus() {
    ...
    bus_offset = (busn & PCIE_ECAM_BUS_MASK) << bus_shift;
    devfn_offset = (devfn & PCIE_ECAM_DEVFN_MASK) << devfn_shift;
    where &= PCIE_ECAM_REG_MASK;

    return base + (bus_offset | devfn_offset | where);
}

For CAM mode, bus_shift is 16, making devfn_shift 8. This places
devfn_offset in bits 8-15. However, PCIE_ECAM_REG_MASK is 0xfff, which
allows the where variable to occupy bits 0-11.

Does this allow extended config space offsets to overlap and bitwise OR
with devfn_offset, inadvertently redirecting accesses to adjacent devices
instead of the intended target?

> +		.read		= pci_generic_config_read,
> +		.write		= pci_generic_config_write,
> +	}
> +};
> +EXPORT_SYMBOL_GPL(pci_generic_cam_ops);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709122446.3151899-1-spersvold@gmail.com?part=1