Re: [PATCH v2 9/9] perf/cxl: Avoid cpumask_of(-1) when no CPU is assigned

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing validation in cxl_pmu_event_init() leads to a severe out-of-bounds memory access via event->cpu.
--

--- Patch [9]: [PATCH v2 9/9] perf/cxl: Avoid cpumask_of(-1) when no CPU is assigned ---
commit a8e317fb50ca3d5fbb66719c1a84b3a5f9c785fc
Author: Dave Jiang <[email protected]>

perf/cxl: Avoid cpumask_of(-1) when no CPU is assigned

This commit fixes an out-of-bounds pointer arithmetic issue in cpumask_show()
caused by cpumask_of(-1) when info->on_cpu is transiently -1 during CPU
hotplug. It reads on_cpu once and emits an empty mask if it is negative.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=1

> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -501,8 +501,17 @@ static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
>  	struct cxl_pmu_info *info = dev_get_drvdata(dev);
> +	int cpu = READ_ONCE(info->on_cpu);
>  
> -	return cpumap_print_to_pagebuf(true, buf, cpumask_of(info->on_cpu));
> +	/*
> +	 * on_cpu is -1 before the first online callback and transiently during
> +	 * cxl_pmu_offline_cpu(). cpumask_of(-1) computes an out-of-bounds
> +	 * pointer, so report an empty mask instead.
> +	 */

[Severity: High]
This problem wasn't introduced by this patch, but since we are handling the
transient -1 state of info->on_cpu here, could the same state cause an
out-of-bounds memory access in cxl_pmu_event_init()?

If perf_event_open() is called concurrently while a CPU hotplug offline
operation (cxl_pmu_offline_cpu()) is temporarily setting info->on_cpu = -1,
the event initialization seems to blindly copy this value:

drivers/perf/cxl_pmu.c:cxl_pmu_event_init() {
    ...
	event->cpu = info->on_cpu;
    ...
}

The core perf subsystem then appears to dereference this -1 CPU index as an
array offset:

kernel/events/core.c:find_get_context() {
    ...
		cpuctx = per_cpu_ptr(&perf_cpu_context, event->cpu);
		ctx = &cpuctx->ctx;
    ...
}

Does passing -1 here cause an out-of-bounds array read that results in a
wild pointer to cpuctx? It looks like the kernel then immediately attempts
to take a spinlock on this wild pointer, which might lead to memory
corruption. Is additional validation needed in cxl_pmu_event_init() to
prevent this?

> +	if (cpu < 0)
> +		return sysfs_emit(buf, "\n");
> +
> +	return cpumap_print_to_pagebuf(true, buf, cpumask_of(cpu));
>  }
>  static DEVICE_ATTR_RO(cpumask);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9