Re: [PATCH v17 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 4 potential issue(s) to consider:

New issues:
- [Medium] sysfs binary read callback incorrectly returns 0 (EOF) for unaligned offsets, breaking partial reads.
- [Medium] Returning -ENODEV for impossible CPUs aborts sequential file reads on sparse CPU topologies.

Pre-existing issues:
- [High] Missing length validation when parsing ACPI _DSM output buffer leads to an out-of-bounds read.
- [High] ACPI _DSM buffer parsing uses an unaligned 64-bit cast and C bitfields, breaking on strict-alignment and big-endian architectures.
--

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

PCI/TPH: Add sysfs binary file to export CPU to steering-tag mapping
    
Add per-device sysfs binary attribute tph_cpu_st to expose ACPI DSM CPU
to steering-tag data to userspace, resolving the concern that VFIO should
not host CPU-to-ST translation interfaces.

> diff --git a/drivers/pci/tph.c b/drivers/pci/tph.c
> index 95280aab4fb51..aca5093e81525 100644
> --- a/drivers/pci/tph.c
> +++ b/drivers/pci/tph.c
> @@ -130,6 +130,29 @@ 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 isn't a bug introduced by this patch, but tph_invoke_dsm() appears to
have an out-of-bounds read.  When parsing the ACPI _DSM output, it
unconditionally dereferences the buffer pointer without validating the
length:

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 firmware returns a buffer smaller than 8 bytes,
could this lead to an out-of-bounds memory read?

This is also a pre-existing issue, but does casting the byte buffer to a u64*
and dereferencing it break on strict-alignment architectures?

Additionally, union st_info uses architecture-dependent C bitfields to
extract data from this 64-bit value.  Could this extract incorrect bits on
big-endian architectures, resulting in corruption of the hardware steering
tags?

[ ... ]

> @@ -619,3 +647,58 @@ bool pcie_tph_supported(struct pci_dev *pdev, bool want_ext)
>  	return pdev->tph_ext_support;
>  }
>  EXPORT_SYMBOL(pcie_tph_supported);
> +
> +static ssize_t tph_cpu_st_read(struct file *filp, struct kobject *kobj,
> +			       const struct bin_attribute *bin_attr, char *buf,
> +			       loff_t off, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +	size_t entry_sz = PCI_TPH_CPU_ST_ENTRY_SZ;
> +	struct pci_tph_cpu_st st;
> +	unsigned int target_cpu;
> +	size_t copy_len;
> +
> +	if (off >= nr_cpu_ids * entry_sz || off % entry_sz != 0)
> +		return 0;

[Severity: Medium]
Does returning 0 here prematurely signal an End-Of-File (EOF) for unaligned
reads?  If a userspace application attempts a short read or reads at an
unaligned offset, should this reject the request with -EINVAL or properly
serve the unaligned data instead of truncating the stream?

> +
> +	target_cpu = off / entry_sz;
> +	if (!cpu_possible(target_cpu))
> +		return -ENODEV;

[Severity: Medium]
Could returning -ENODEV here abort sequential file reads on systems with
sparse CPU topologies?  If sequential readers (e.g., using cat or hexdump)
hit this error at the first missing CPU, they might fail to retrieve valid
steering tags for any CPUs located after the topology gap.  Would it be
better to skip the missing CPU by returning a zeroed entry instead?

> +
> +	get_cpu_all_st(pdev, target_cpu, &st);
> +
> +	copy_len = min_t(size_t, entry_sz, count);
> +	memcpy(buf, &st, copy_len);
> +
> +	return copy_len;
> +}

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