lkml.org 
[lkml]   [2020]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task
From
Date


On 7/30/2020 8:58 AM, peterz@infradead.org wrote:
> On Thu, Jul 30, 2020 at 05:38:15AM -0700, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> The counter value of a perf task may leak to another RDPMC task.
>
> Sure, but nowhere did you explain why that is a problem.
>
>> The RDPMC instruction is only available for the X86 platform. Only apply
>> the fix for the X86 platform.
>
> ARM64 can also do it, although I'm not sure what the current state of
> things is here.
>
>> After applying the patch,
>>
>> $ taskset -c 0 ./rdpmc_read_all_counters
>> index 0x0 value 0x0
>> index 0x1 value 0x0
>> index 0x2 value 0x0
>> index 0x3 value 0x0
>>
>> index 0x0 value 0x0
>> index 0x1 value 0x0
>> index 0x2 value 0x0
>> index 0x3 value 0x0
>
> You forgot about:
>
> - telling us why it's a problem,

The non-privileged RDPMC user can get the counter information from other
perf users. It is a security issue. I will add it in the next version.

> - telling us how badly it affects performance.

I once did performance test on a HSX machine. There is no notable slow
down with the patch. I will add the performance data in the next version.

>
> I would feel much better if we only did this on context switches to
> tasks that have RDPMC enabled.

AFAIK, at least for X86, we can only enable/disable RDPMC globally.
How can we know if a specific task that have RDPMC enabled/disabled?

>
> So on del() mark the counter dirty (if we don't already have state that
> implies this), but don't WRMSR. And then on
> __perf_event_task_sched_in(), _after_ programming the new tasks'
> counters, check for inactive dirty counters and wipe those -- IFF RDPMC
> is on for that task.
>

The generic code doesn't have counters' information. It looks like we
need to add a new callback to cleanup the dirty counters as below.

In the specific implementation of pmu_cleanup(), we can check and wipe
all inactive dirty counters.

Is it OK?

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 1cbf57dc2ac8..3daaf0a7746d 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -3774,6 +3781,15 @@ static void perf_event_context_sched_in(struct
perf_event_context *ctx,
if (!RB_EMPTY_ROOT(&ctx->pinned_groups.tree))
cpu_ctx_sched_out(cpuctx, EVENT_FLEXIBLE);
perf_event_sched_in(cpuctx, ctx, task);
+
+ /*
+ * Some leftovers from the previous task may still exist on the unused
+ * counters. The new task may illegally read the counters, e.g. via
+ * RDPMC. The information from the previous task will be leaked. Clean
+ * up the PMU before enabling it.
+ */
+ if (ctx->pmu->pmu_cleanup)
+ ctx->pmu->pmu_cleanup(pmu);
perf_pmu_enable(ctx->pmu);

unlock:


Thanks,
Kan

\
 
 \ /
  Last update: 2020-07-30 17:55    [W:0.101 / U:0.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site