Re: [PATCH v7 6/6] PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unchecked NULL pointer dereference in `pci_dev_has_memory_bars()` when evaluating the upstream bridge of a Downstream Port.
--

commit ed8b20f412e18730ccf6172bf08ed537dfd42012
Author: Wei Wang <wei.w.wang@hotmail.com>

PCI: Add the enhanced ACS controls check to pci_acs_flags_enabled()

This commit adds enhanced ACS controls checks introduced by PCIe Gen 5 to
ensure better device isolation. It verifies specific ACS controls on devices
that support the PCI_ACS_ECAP capability.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0c15de0aedb2..b684ae4e92b5e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -3551,6 +3551,53 @@ void pci_configure_ari(struct pci_dev *dev)
>  	}
>  }
>  
> +static bool pci_dev_has_memory_bars(struct pci_dev *pdev)
> +{
> +	int i;
> +
> +	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
> +		if (pci_resource_flags(pdev, i) & IORESOURCE_MEM)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static bool pci_acs_ecap_enabled(struct pci_dev *pdev, u16 ctrl)
> +{
> +	/*
> +	 * For ACS DSP/USP Memory Target Access Control, either Request
> +	 * Redirect or Request Blocking must be enabled to enforce isolation.
> +	 * According to PCIe spec 7.0, the DSP Memory Target Access is
> +	 * applicable to both Root Ports and Switch Downstream Ports that have
> +	 * applicable Memory BAR space to protect. So if the device does not
> +	 * have a Memory BAR, it skips the check.
> +	 */
> +	if (pci_dev_has_memory_bars(pdev) &&
> +	    FIELD_GET(PCI_ACS_DMAC_MASK, ctrl) != PCI_ACS_MAC_RB &&
> +	    FIELD_GET(PCI_ACS_DMAC_MASK, ctrl) != PCI_ACS_MAC_RR)
> +		return false;
> +
> +	if (pci_pcie_type(pdev) == PCI_EXP_TYPE_DOWNSTREAM) {
> +		/*
> +		 * The USP Memory Target Access is only applicable to
> +		 * downstream ports that have applicable Memory BAR space in
> +		 * the Switch Upstream Port to protect.
> +		 */
> +		if (pci_dev_has_memory_bars(pci_upstream_bridge(pdev)) &&

Could pci_upstream_bridge() return NULL here?

If the device resides on a root bus, which can sometimes occur in certain
virtualized environments or hardware topologies, pci_upstream_bridge() will
return NULL.

If that happens, would pci_dev_has_memory_bars() dereference the NULL
pointer when evaluating pci_resource_flags()?

> +		    FIELD_GET(PCI_ACS_UMAC_MASK, ctrl) != PCI_ACS_MAC_RB &&
> +		    FIELD_GET(PCI_ACS_UMAC_MASK, ctrl) != PCI_ACS_MAC_RR)
> +			return false;
> +
> +		/* PCI_ACS_URRC is applicable to Downstream Ports only. */
> +		if (!(ctrl & PCI_ACS_URRC))
> +			return false;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB439385689A32A1DDA9CEABE1DC3F2@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=6