[PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

The counter value of a perf task may leak to another RDPMC task.
For example, a perf stat task as below is running on CPU 0.

    perf stat -e 'branches,cycles' -- taskset -c 0 ./workload

In the meantime, an RDPMC task, which is also running on CPU 0, may read
the GP counters periodically. (The RDPMC task creates a fixed event,
but read four GP counters.)

    $ taskset -c 0 ./rdpmc_read_all_counters
    index 0x0 value 0x8001e5970f99
    index 0x1 value 0x8005d750edb6
    index 0x2 value 0x0
    index 0x3 value 0x0

    index 0x0 value 0x8002358e48a5
    index 0x1 value 0x8006bd1e3bc9
    index 0x2 value 0x0
    index 0x3 value 0x0

The counter value of the perf stat task leaks to the RDPMC task because
perf never clears the counter when it's stopped.

A counter/event stops for the following cases:
- Schedule the monitored task, e.g., context switch or task exit;
- Adjust the event period, e.g., Throttle, in frequency mode;
- Update the event's address range filters.

For the first case, following tasks may reuse the counter. To
prevent the leak, the counter has to be reset, when the event is
scheduled out. The del() is eventually invoked for the schedule out.
The counter should be reset in x86_pmu_del().

For the rest of the cases, the counter/event is temporarily stopped and
will be restarted soon. Other tasks don't have a chance to reuse the
counter. Reset the counter is not necessary.

The RDPMC instruction is only available for the X86 platform. Only apply
the fix for the X86 platform.

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

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 arch/x86/events/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 01ba5fec5765..72b2c7e1bb20 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1499,6 +1499,13 @@ static void x86_pmu_del(struct perf_event *event, int flags)
 	 */
 	x86_pmu_stop(event, PERF_EF_UPDATE);
 
+	/*
+	 * The counter value may leak to an RDPMC task.
+	 * Clear the counter if the userspace RDPMC usage is enabled.
+	 */
+	if (READ_ONCE(x86_pmu.attr_rdpmc))
+		wrmsrl(event->hw.event_base, 0);
+
 	for (i = 0; i < cpuc->n_events; i++) {
 		if (event == cpuc->event_list[i])
 			break;
-- 
2.17.1

Re: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[peterz]

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,
 - telling us how badly it affects performance.

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

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.

Re: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[Liang, Kan]

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

Re: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[peterz]

On Thu, Jul 30, 2020 at 11:54:35AM -0400, Liang, Kan wrote:
> 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.

You don't know what it counted and you don't know the offset, what can
you do with it?

> >   - 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.

It's still up to [4..8]+[3,4] extra WRMSRs per context switch, that's pretty naf.

> > 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?

It has mm->context.pref_rdpmc_allowed non-zero, go read x86_pmu_event_{,un}mapped().
Without that CR4.PCE is 0 and RDPMC won't work, which is most of the
actual tasks.

Arguably we should have perf_mmap_open() check if 'event->hw.target ==
current', because without that RDPMC is still pointless.

> > 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.

What about pmu::sched_task(), can't we rejig that a little?

The way I'm reading it now, it's like we iterate the task context for
calling perf_event_context_sched_*(), and then iterate a cpuctx list to
find cpuctx->task_ctx, which would be the exact same contexts we've just
iterated.

So can't we pull the pmu::sched_task() call into
perf_event_context_sched_*() ? That would save a round of
pmu_disable/enable() too afaict.

Re: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[peterz]

On Thu, Jul 30, 2020 at 06:44:26PM +0200, peterz@infradead.org wrote:
> Arguably we should have perf_mmap_open() check if 'event->hw.target ==
> current', because without that RDPMC is still pointless.

event->hw.target->mm == current->mm, I suppose, otherwise accounting
goes wobbly real fast.

Re: [PATCH] perf/x86: Reset the counter to prevent the leak for a RDPMC task

[Liang, Kan]

On 7/30/2020 12:44 PM, peterz@infradead.org wrote:
> On Thu, Jul 30, 2020 at 11:54:35AM -0400, Liang, Kan wrote:
>> 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.
> 
> You don't know what it counted and you don't know the offset, what can
> you do with it?

We cannot guarantee that an attacker doesn't know what the other thread 
is doing. Once they know the event name, they may take advantage of the 
perfmon counters to attack on a cryptosystem.

Here is one paper I googled. https://dl.acm.org/doi/pdf/10.1145/3156015
It mentioned that some events, e.g., cache misses and branch miss, can 
be used as a side channel to attack on cryptosystems.

There are potential security issues.
> 
>>>    - 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.
> 
> It's still up to [4..8]+[3,4] extra WRMSRs per context switch, that's pretty naf.

I will do more performance test on a ICL with full GP counters and fixed 
counters enabled.

> 
>>> 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?
> 
> It has mm->context.pref_rdpmc_allowed non-zero, go read x86_pmu_event_{,un}mapped().
> Without that CR4.PCE is 0 and RDPMC won't work, which is most of the
> actual tasks.
> 

Thanks for pointing it out.

I think I can use event->mmap_count and PERF_X86_EVENT_RDPMC_ALLOWED to 
check whether the events of the task have RDPMC enabled.

> Arguably we should have perf_mmap_open() check if 'event->hw.target ==
> current', because without that RDPMC is still pointless. >
>>> 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.
> 
> What about pmu::sched_task(), can't we rejig that a little?
> 
> The way I'm reading it now, it's like we iterate the task context for
> calling perf_event_context_sched_*(), and then iterate a cpuctx list to
> find cpuctx->task_ctx, which would be the exact same contexts we've just
> iterated.
> 
> So can't we pull the pmu::sched_task() call into
> perf_event_context_sched_*() ? That would save a round of
> pmu_disable/enable() too afaict.
> 

I think it's doable. I will do more test.

Thanks,
Kan