linux-kernel - Re: [PATCH v12 4/5] drivers/perf: add DesignWare PCIe PMU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241206165457.GA3101599@bhelgaas>
Date: Fri, 6 Dec 2024 10:54:57 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Shuai Xue <xueshuai@...ux.alibaba.com>
Cc: ilkka@...amperecomputing.com, kaishen@...ux.alibaba.com,
	yangyicong@...wei.com, will@...nel.org, Jonathan.Cameron@...wei.com,
	baolin.wang@...ux.alibaba.com, robin.murphy@....com,
	chengyou@...ux.alibaba.com, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-pci@...r.kernel.org,
	rdunlap@...radead.org, mark.rutland@....com,
	zhuo.song@...ux.alibaba.com, renyu.zj@...ux.alibaba.com
Subject: Re: [PATCH v12 4/5] drivers/perf: add DesignWare PCIe PMU driver

On Fri, Dec 08, 2023 at 10:56:51AM +0800, Shuai Xue wrote:
> This commit adds the PCIe Performance Monitoring Unit (PMU) driver support
> for T-Head Yitian SoC chip. Yitian is based on the Synopsys PCI Express
> Core controller IP which provides statistics feature. The PMU is a PCIe
> configuration space register block provided by each PCIe Root Port in a
> Vendor-Specific Extended Capability named RAS D.E.S (Debug, Error
> injection, and Statistics).

> +#define DWC_PCIE_VSEC_RAS_DES_ID		0x02

> +static const struct dwc_pcie_vendor_id dwc_pcie_vendor_ids[] = {
> +	{.vendor_id = PCI_VENDOR_ID_ALIBABA },
> +	{} /* terminator */
> +};

> +static bool dwc_pcie_match_des_cap(struct pci_dev *pdev)
> +{
> +	const struct dwc_pcie_vendor_id *vid;
> +	u16 vsec;
> +	u32 val;
> +
> +	if (!pci_is_pcie(pdev) || !(pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT))
> +		return false;
> +
> +	for (vid = dwc_pcie_vendor_ids; vid->vendor_id; vid++) {
> +		vsec = pci_find_vsec_capability(pdev, vid->vendor_id,
> +						DWC_PCIE_VSEC_RAS_DES_ID);

This looks wrong to me, and it promotes a misunderstanding of how VSEC
Capabilities work.  The VSEC ID is defined by the vendor, so we have
to check both the Vendor ID and the VSEC ID before we know what this
VSEC Capability is.

In this patch, we only find a VSEC Capability that matches
(PCI_VENDOR_ID_ALIBABA, DWC_PCIE_VSEC_RAS_DES_ID), but as of
v6.13-rc1, it finds all of these:

  (PCI_VENDOR_ID_ALIBABA, DWC_PCIE_VSEC_RAS_DES_ID)
  (PCI_VENDOR_ID_AMPERE, DWC_PCIE_VSEC_RAS_DES_ID)
  (PCI_VENDOR_ID_QCOM, DWC_PCIE_VSEC_RAS_DES_ID)

There is no assurance that DWC_PCIE_VSEC_RAS_DES_ID means the same
thing to Alibaba, Ampere, and Qualcomm because each company defines
what 0x02 means to it.  PCIe r6.0, sec 7.9.5 for details.

I alluded to this earlier [1] and suggested that these devices should
have used a Designated Vendor-Specific (DVSEC) Capability instead of a 
Vendor-Specific (VSEC) Capability.

But since they didn't, I think the dwc_pcie_vendor_ids[] table is a
dangerous way to work around this because it suggests that all we have
to do is add new vendors to that table.

I think the table should be extended to contain the Vendor ID, *and*
the VSEC ID, *and* the VSEC Rev used by that vendor, i.e., it should
look like this:

  struct dwc_pcie_pmu_vsec {
    u16 vendor_id;
    u16 vsec_id;
    u8 vsec_rev;
  };

  struct dwc_pcie_pmu_vsec dwc_pcie_pmu_vsec_ids[] = {
    { .vendor_id = PCI_VENDOR_ID_ALIBABA,
      .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
    { .vendor_id = PCI_VENDOR_ID_AMPERE,
      .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
    { .vendor_id = PCI_VENDOR_ID_QCOM,
      .vsec_id = DWC_PCIE_VSEC_RAS_DES_ID, .vsec_rev = 0x4 },
    {}
  };

This *looks* the same, but it's not, because it makes it obvious that
the VSEC ID and VSEC Rev are defined separately by each vendor.  It's
just a lucky coincidence that they happen to be the same for these
vendors.

> +		if (vsec)
> +			break;
> +	}
> +	if (!vsec)
> +		return false;
> +
> +	pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> +	if (PCI_VNDR_HEADER_REV(val) != 0x04)
> +		return false;
> +
> +	pci_dbg(pdev,
> +		"Detected PCIe Vendor-Specific Extended Capability RAS DES\n");
> +	return true;
> +}

> +static int dwc_pcie_pmu_probe(struct platform_device *plat_dev)
> +{
> +	struct pci_dev *pdev = plat_dev->dev.platform_data;
> +	struct dwc_pcie_pmu *pcie_pmu;
> +	char *name;
> +	u32 bdf, val;
> +	u16 vsec;
> +	int ret;
> +
> +	vsec = pci_find_vsec_capability(pdev, pdev->vendor,
> +					DWC_PCIE_VSEC_RAS_DES_ID);
> +     pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);

Nit: "val" is never used, so why read it?

This looks even more wrong, because this matches ANY VSEC Capability
from ANY vendor that happens to be numbered DWC_PCIE_VSEC_RAS_DES_ID.

I know this is indirectly qualified by the check above in
dwc_pcie_match_des_cap(), but duplicating this here just spreads the
confusion about how to interpret VSEC IDs.

I suggest updating dwc_pcie_match_des_cap() to iterate through the
dwc_pcie_pmu_vsec_ids[] table and return the capability offset so you
can call it from here.

Bjorn

[1] https://lore.kernel.org/r/20231012162512.GA1069387@bhelgaas