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

[Shanker Donthineni]

Hi Robin,

On 10/26/23 11:39, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> [ 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.
> 

It appears that there is already a private pointer named 'pmu_private' in
the 'perf_event' structure. I will submit version 2 of the code with this
modification.

--- a/drivers/perf/resctrl_pmu.c
+++ b/drivers/perf/resctrl_pmu.c
@@ -66,7 +66,6 @@ static struct rdt_resource *resctrl_event_get_resource(u16 event_num)

  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;

@@ -74,7 +73,7 @@ static void resctrl_pmu_event_destroy(struct perf_event *event)
         if (!r)
                 return;

-       resctrl_arch_mon_ctx_free(r, event_num, hwc->idx);
+       resctrl_arch_mon_ctx_free(r, event_num, event->pmu_private);
  }

  static int resctrl_pmu_event_init(struct perf_event *event)
@@ -144,8 +143,8 @@ static int resctrl_pmu_event_init(struct perf_event *event)
                         return -EINVAL;
         }

-       hwc->idx = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num);
-       if (hwc->idx == -ENOSPC)
+       event->pmu_private = resctrl_arch_mon_ctx_alloc_no_wait(r, event_num);
+       if (IS_ERR_OR_NULL(event->pmu_private))
                 return -ENOSPC;
         event->destroy = resctrl_pmu_event_destroy;
         local64_set(&hwc->prev_count, 0);
@@ -183,7 +182,8 @@ static void resctrl_pmu_event_update(struct perf_event *event)
                 prev = local64_read(&hwc->prev_count);

                 err = resctrl_arch_rmid_read(r, d, closid, rmid,
-                                            event_num, &now, hwc->idx);
+                                            event_num, &now,
+                                            event->pmu_private);




> 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);