[PATCH 1/1] drivers/perf: Fix kernel panic due to the invalid mon_ctx pointer

[Robin Murphy]

[ Since I made the mistake of looking... ]

On 26/10/2023 3:29 pm, Shanker Donthineni wrote:
> The return pointer from the resctrl_arch_mon_ctx_alloc_no_wait() function
> is saved in a 32-bit variable 'hwc->idx,' which results in the loss of
> the upper 32 bits. This, in turn, triggers a kernel panic when attempting
> to access a corrupted pointer.
> 
> This patch addresses the issue by utilizing the IDR data structure to
> keep track of a 32-bit value associated with a pointer (64-bit value).

This seems unnecessaraily complicated - there's plenty of space in that 
anonymous hw_perf_event "driver data" union (I know I cram quite a lot 
in there for arm-cmn) so it should just be a case of picking a different 
appropriately-sized field to (ab)use.

Thanks,
Robin.

> Signed-off-by: Shanker Donthineni <sdonthineni at nvidia.com>
> ---
>   drivers/perf/resctrl_pmu.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/perf/resctrl_pmu.c b/drivers/perf/resctrl_pmu.c
> index 99a2b90b5d83..68cda619da48 100644
> --- a/drivers/perf/resctrl_pmu.c
> +++ b/drivers/perf/resctrl_pmu.c
> @@ -6,6 +6,8 @@
>   #include <linux/perf_event.h>
>   #include <linux/platform_device.h>
>   #include <linux/resctrl.h>
> +#include <linux/mutex.h>
> +#include <linux/idr.h>
>   
>   struct resctrl_pmu {
>   	struct pmu pmu;
> @@ -25,6 +27,9 @@ RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 7);
>   RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(domain, config, 16, 23);
>   RESCTRL_PMU_EVENT_ATTR_EXTRACTOR(resctrl_id, config1, 0, 63);
>   
> +static DEFINE_MUTEX(resctrl_pmu_lock);
> +static DEFINE_IDR(resctrl_pmu_idr);
> +
>   static void resctrl_pmu_do_nothing(struct pmu *pmu)
>   {
>   }
> @@ -69,12 +74,17 @@ static void resctrl_pmu_event_destroy(struct perf_event *event)
>   	struct hw_perf_event *hwc = &event->hw;
>   	u16 event_num = get_event(event);
>   	struct rdt_resource *r;
> +	void *mon_ctx;
>   
>   	r = resctrl_event_get_resource(event_num);
>   	if (!r)
>   		return;
>   
> -	resctrl_arch_mon_ctx_free(r, event_num, hwc->idx);
> +	mutex_lock(&resctrl_pmu_lock);
> +	mon_ctx = idr_remove(&resctrl_pmu_idr, hwc->idx);
> +	mutex_unlock(&resctrl_pmu_lock);
> +
> +	resctrl_arch_mon_ctx_free(r, event_num, mon_ctx);
>   }
>   
>   static int resctrl_pmu_event_init(struct perf_event *event)
> @@ -87,6 +97,7 @@ static int resctrl_pmu_event_init(struct perf_event *event)
>   	struct rdt_domain *d;
>   	u16 event_num, domain_id;
>   	u32 closid, rmid;
> +	void *mon_ctx;
>   	int err;
>   	u64 id;
>   
> @@ -144,7 +155,12 @@ static int resctrl_pmu_event_init(struct perf_event *event)
>   			return -EINVAL;
>   	}
>   
> -	hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num);
> +	mon_ctx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num);
> +
> +	mutex_lock(&resctrl_pmu_lock);
> +	hwc->idx = idr_alloc(&resctrl_pmu_idr, mon_ctx, 0, INT_MAX, GFP_KERNEL);
> +	mutex_unlock(&resctrl_pmu_lock);
> +
>   	if (hwc->idx == -ENOSPC)
>   		return -ENOSPC;
>   	event->destroy = resctrl_pmu_event_destroy;
> @@ -162,6 +178,7 @@ static void resctrl_pmu_event_update(struct perf_event *event)
>   	struct rdt_resource *r;
>   	struct rdt_domain *d;
>   	u32 closid, rmid;
> +	void *mon_ctx;
>   	int err;
>   
>   	event_num = get_event(event);
> @@ -179,11 +196,15 @@ static void resctrl_pmu_event_update(struct perf_event *event)
>   	if (!cpumask_test_cpu(smp_processor_id(), &d->cpu_mask))
>   		return;
>   
> +	mutex_lock(&resctrl_pmu_lock);
> +	mon_ctx = idr_find(&resctrl_pmu_idr, hwc->idx);
> +	mutex_unlock(&resctrl_pmu_lock);
> +
>   	do {
>   		prev = local64_read(&hwc->prev_count);
>   
>   		err = resctrl_arch_rmid_read(r, d, closid, rmid,
> -					     event_num, &now, hwc->idx);
> +					     event_num, &now, mon_ctx);
>   		if (err)
>   			return;
>   	} while (local64_cmpxchg(&hwc->prev_count, prev, now) != prev);