Re: [PATCH RESEND v18 08/12] PCI/TPH: Add sysfs binary file to export CPU to steering-tag mapping

[sashiko-bot@kernel.org]

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

New issues:
- [High] Using `pci_read_config_dword()` to read the PCIe capability register `PCI_EXP_DEVCAP2` reads from the wrong offset in the base PCI configuration space.

Pre-existing issues:
- [High] Missing ACPI buffer length check before dereferencing u64 data in `tph_invoke_dsm()`, leading to a potential out-of-bounds read.
--

commit a6ad2b09ffbe79acf9a6193d9d9e38bc463af8b8
Author: Chengwen Feng <fengchengwen@huawei.com>

PCI/TPH: Add sysfs binary file to export CPU to steering-tag mapping

This commit adds a per-Root-Port sysfs binary attribute tph_cpu_st to expose
the ACPI DSM CPU-to-ST mapping to userspace. It supports unaligned partial
reads and implements dynamic visibility rules to ensure it is only exposed
on PCIe Root Ports that support TPH and provide a valid ACPI DSM.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index c080cf5401a31..bb6f24eccafcc 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c

[ ... ]

> @@ -130,8 +131,47 @@ static acpi_status tph_invoke_dsm(acpi_handle handle, u32 cpu_uid,
>  
>  	return AE_OK;
>  }
> +
> +static int tph_get_cpu_st_info(struct pci_dev *pdev, unsigned int cpu,
> +			       union st_info *info)
> +{
> +	acpi_handle rp_acpi_handle;
> +	struct pci_dev *rp;
> +	u32 cpu_uid;
> +	int ret;
> +
> +	ret = acpi_get_cpu_uid(cpu, &cpu_uid);
> +	if (ret != 0)
> +		return ret;
> +
> +	rp = pcie_find_root_port(pdev);
> +	if (!rp || !rp->bus || !rp->bus->bridge)
> +		return -ENODEV;
> +
> +	rp_acpi_handle = ACPI_HANDLE(rp->bus->bridge);
> +	if (tph_invoke_dsm(rp_acpi_handle, cpu_uid, info) != AE_OK)

[Severity: High]
This is a pre-existing issue, but does tph_invoke_dsm() lack an ACPI buffer
length check before dereferencing u64 data?

Looking at tph_invoke_dsm(), it appears to unconditionally cast and dereference
the returned ACPI buffer pointer:

drivers/pci/tph.c:tph_invoke_dsm() {
	...
	if (out_obj->type != ACPI_TYPE_BUFFER) {
		ACPI_FREE(out_obj);
		return AE_ERROR;
	}

	st_out->value = *((u64 *)(out_obj->buffer.pointer));
	...
}

If a buggy or malicious ACPI BIOS returns a buffer smaller than 8 bytes, could
this result in an out-of-bounds memory read?

Although pre-existing, exposing this via the new tph_cpu_st sysfs attribute
might allow a root user to systematically trigger this evaluation across all
possible CPUs.

> +		return -EINVAL;
> +
> +	return 0;
> +}

[ ... ]

> @@ -615,3 +661,78 @@ bool pcie_tph_supported(struct pci_dev *pdev, bool want_ext)

[ ... ]

> +static umode_t tph_cpu_st_attr_is_visible(struct kobject *kobj,
> +					  const struct bin_attribute *a, int n)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +	bool is_root_port = pci_is_pcie(pdev) &&
> +				pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT;
> +	u32 devcap2 = 0;
> +
> +	if (!is_root_port)
> +		return 0;
> +
> +	pci_read_config_dword(pdev, PCI_EXP_DEVCAP2, &devcap2);

[Severity: High]
Does this code read from the wrong offset in the base PCI configuration space?

PCI_EXP_DEVCAP2 is an offset relative to the PCIe Capability structure, but
pci_read_config_dword() takes an absolute offset in the base PCI config space.

Should this use pcie_capability_read_dword() instead so the attribute
visibility is determined correctly?

> +	if (!(devcap2 & PCI_EXP_DEVCAP2_TPH_COMP_MASK))
> +		return 0;
> +
> +	if (!tph_dsm_supported(pdev))
> +		return 0;
> +
> +	return a->attr.mode;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623085048.51139-1-fengchengwen@huawei.com?part=8