Re: [PATCH V2 6/7] perf/x86/intel/uncore: Fix uncore_die_to_cpu() for offline dies

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but in `pmu_alloc_topology()`, if the memory allocation for `topology[die]` fails, the error cleanup loop dereferences the newly-NULL pointer, causing a kernel crash.
- [High] This problem wasn't introduced by this patch, but there is an unprotected list traversal of PCI buses in `uncore_die_to_segment()` leading to a potential Use-After-Free if a PCI bus is hot-removed.
--

commit ff1f5cc12e40244014f762aa4e444d2ef1d867a3
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Fix uncore_die_to_cpu() for offline dies

If the die is offline when uncore_die_to_cpu() is called, it silently
returns 0, which is misleading. This patch modifies the function to return
-1 in this case, indicating that all CPUs on the die are offline. It also
opportunistically replaces -EPERM with -ENODEV.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index 22256ded2d67d..4b3a1fa5b41b7 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -91,7 +91,7 @@ int uncore_device_to_die(struct pci_dev *dev)
>   */
>  int uncore_die_to_cpu(int die)
>  {
> -	int res = 0, cpu;
> +	int res = -1, cpu;
>  
>  	for_each_online_cpu(cpu) {
>  		if (topology_logical_die_id(cpu) == die) {

[ ... ]

> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 772b78237424c..334dc384b5b93 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -3772,12 +3772,17 @@ static void pmu_free_topology(struct intel_uncore_type *type)
>  static int skx_pmu_get_topology(struct intel_uncore_type *type,
>  				 int (*topology_cb)(struct intel_uncore_type*, int, int, u64))
>  {
> -	int die, ret = -EPERM;
> +	int die, ret = -ENODEV;
>  	u64 cpu_bus_msr;
> +	int cpu;
>  
>  	cpus_read_lock();
>  	for (die = 0; die < uncore_max_dies(); die++) {
> -		ret = skx_msr_cpu_bus_read(uncore_die_to_cpu(die), &cpu_bus_msr);
> +		cpu = uncore_die_to_cpu(die);
> +		if (cpu == -1)
> +			continue;
> +
> +		ret = skx_msr_cpu_bus_read(cpu, &cpu_bus_msr);
>  		if (ret)
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but later in this function, we
call uncore_die_to_segment() which iterates over PCI root buses using
pci_find_next_bus():

arch/x86/events/intel/uncore.c:uncore_die_to_segment() {
	struct pci_bus *bus = NULL;

	/* Find first pci bus which attributes to specified die. */
	while ((bus = pci_find_next_bus(bus)) &&
	       (die != uncore_pcibus_to_dieid(bus)))
		;

	return bus ? pci_domain_nr(bus) : -EINVAL;
}

The pci_find_next_bus() function acquires and releases pci_bus_sem internally,
returning a raw bus pointer without holding a lock or taking a reference count.

If an unrelated hot-pluggable PCI root bus is hot-removed concurrently, could
the bus pointer be freed before the next iteration?

Passing this freed pointer back to pci_find_next_bus() could cause a
use-after-free when accessing from->node.next. 

While cpus_read_lock() is held in skx_pmu_get_topology(), does it prevent
concurrent hotplug of unrelated PCI root buses?

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference in pmu_alloc_topology() in this same file.

If kzalloc_objs() for topology[die] fails, the code jumps to the clear label:

arch/x86/events/intel/uncore_snbep.c:pmu_alloc_topology() {
	...
	for (die = 0; die < uncore_max_dies(); die++) {
		topology[die] = kzalloc_objs(**topology, type->num_boxes);
		if (!topology[die])
			goto clear;
	...
clear:
	for (; die >= 0; die--) {
		for (idx = 0; idx < type->num_boxes; idx++)
			kfree(topology[die][idx].untyped);
		kfree(topology[die]);
	...
}

At the clear label, the loop variable die still points to the index that
failed allocation, so topology[die] is NULL.

Does the cleanup loop's attempt to execute kfree(topology[die][idx].untyped)
evaluate to (topology[die] + idx)->untyped and crash the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527151154.130505-1-zide.chen@intel.com?part=6