Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Peter Zijlstra]

On Fri, May 19, 2017 at 06:19:12PM +0800, Jin Yao wrote:
> When doing sampling without PEBS
> 
> perf record -e cycles:u ...
> 
> On workloads that do a lot of kernel entry/exits we see kernel
> samples, even though :u is specified. This is due to skid existing.
> 
> This is a security issue because it can leak kernel addresses even
> though kernel sampling support is disabled.
> 
> The patch drops the kernel samples if exclude_kernel is specified.
> 
> For example, test on Haswell desktop.
> 
> perf record -e cycles:u <mgen>
> perf report --stdio
> 
> Before patch applied:
> 
>     99.77%  mgen     mgen              [.] buf_read
>      0.20%  mgen     mgen              [.] rand_buf_init
>      0.01%  mgen     [kernel.vmlinux]  [k] apic_timer_interrupt


> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 580b60f..e6745e1 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1463,6 +1463,12 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
>  		if (!x86_perf_event_set_period(event))
>  			continue;
>  
> +		/*
> +		 * For security, drop the skid kernel samples.
> +		 */
> +		if (skid_kernel_samples(event, regs))
> +			continue;
> +
>  		if (perf_event_overflow(event, &data, regs))
>  			x86_pmu_stop(event, 0);
>  	}
> @@ -1679,6 +1685,24 @@ ssize_t events_ht_sysfs_show(struct device *dev, struct device_attribute *attr,
>  			pmu_attr->event_str_noht);
>  }
>  
> +bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs)
> +{
> +	u64 ip;
> +
> +	/*
> +	 * Without PEBS, we may get kernel samples even though
> +	 * exclude_kernel is specified due to skid in sampling.
> +	 */
> +	if ((event->attr.exclude_kernel) &&
> +	    (event->attr.sample_type & PERF_SAMPLE_IP)) {
> +		ip = perf_instruction_pointer(regs);
> +		if (kernel_ip(ip))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
>  EVENT_ATTR(instructions,		INSTRUCTIONS		);
>  EVENT_ATTR(cache-references,		CACHE_REFERENCES	);


I would much rather see this in generic code, somewhere around
__perf_event_overflow() I suppose. That would retain proper accounting
for the interrupt rate etc..

Also it would work for all architectures. Because I'm thinking more than
just x86 will suffer from skid.

If you're really worried, I suppose you can put it behind a PERF_PMU_CAP
flag or something.

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Will Deacon]

On Fri, May 19, 2017 at 11:29:05AM +0200, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 06:19:12PM +0800, Jin Yao wrote:
> > +bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs)
> > +{
> > +	u64 ip;
> > +
> > +	/*
> > +	 * Without PEBS, we may get kernel samples even though
> > +	 * exclude_kernel is specified due to skid in sampling.
> > +	 */
> > +	if ((event->attr.exclude_kernel) &&
> > +	    (event->attr.sample_type & PERF_SAMPLE_IP)) {
> > +		ip = perf_instruction_pointer(regs);
> > +		if (kernel_ip(ip))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
> >  EVENT_ATTR(instructions,		INSTRUCTIONS		);
> >  EVENT_ATTR(cache-references,		CACHE_REFERENCES	);
> 
> 
> I would much rather see this in generic code, somewhere around
> __perf_event_overflow() I suppose. That would retain proper accounting
> for the interrupt rate etc..
> 
> Also it would work for all architectures. Because I'm thinking more than
> just x86 will suffer from skid.

Yes, I think this will affect arm/arm64 too (and probably others that rely
on irqs for sampling the regs).

Will

[PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin Yao]

When doing sampling without PEBS

perf record -e cycles:u ...

On workloads that do a lot of kernel entry/exits we see kernel
samples, even though :u is specified. This is due to skid existing.

This is a security issue because it can leak kernel addresses even
though kernel sampling support is disabled.

The patch drops the kernel samples if exclude_kernel is specified.

For example, test on Haswell desktop.

perf record -e cycles:u <mgen>
perf report --stdio

Before patch applied:

    99.77%  mgen     mgen              [.] buf_read
     0.20%  mgen     mgen              [.] rand_buf_init
     0.01%  mgen     [kernel.vmlinux]  [k] apic_timer_interrupt
     0.00%  mgen     mgen              [.] last_free_elem
     0.00%  mgen     libc-2.23.so      [.] __random_r
     0.00%  mgen     libc-2.23.so      [.] _int_malloc
     0.00%  mgen     mgen              [.] rand_array_init
     0.00%  mgen     [kernel.vmlinux]  [k] page_fault
     0.00%  mgen     libc-2.23.so      [.] __random
     0.00%  mgen     libc-2.23.so      [.] __strcasestr
     0.00%  mgen     ld-2.23.so        [.] strcmp
     0.00%  mgen     ld-2.23.so        [.] _dl_start
     0.00%  mgen     libc-2.23.so      [.] sched_setaffinity@@GLIBC_2.3.4
     0.00%  mgen     ld-2.23.so        [.] _start

We can see kernel symbols apic_timer_interrupt and page_fault.

After patch applied:

    99.79%  mgen     mgen           [.] buf_read
     0.19%  mgen     mgen           [.] rand_buf_init
     0.00%  mgen     libc-2.23.so   [.] __random_r
     0.00%  mgen     mgen           [.] rand_array_init
     0.00%  mgen     mgen           [.] last_free_elem
     0.00%  mgen     libc-2.23.so   [.] vfprintf
     0.00%  mgen     libc-2.23.so   [.] rand
     0.00%  mgen     libc-2.23.so   [.] __random
     0.00%  mgen     libc-2.23.so   [.] _int_malloc
     0.00%  mgen     libc-2.23.so   [.] _IO_doallocbuf
     0.00%  mgen     ld-2.23.so     [.] do_lookup_x
     0.00%  mgen     ld-2.23.so     [.] open_verify.constprop.7
     0.00%  mgen     ld-2.23.so     [.] _dl_important_hwcaps
     0.00%  mgen     libc-2.23.so   [.] sched_setaffinity@@GLIBC_2.3.4
     0.00%  mgen     ld-2.23.so     [.] _start

There are only userspace symbols.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 arch/x86/events/core.c       | 24 ++++++++++++++++++++++++
 arch/x86/events/intel/core.c |  6 ++++++
 arch/x86/events/perf_event.h |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 580b60f..e6745e1 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1463,6 +1463,12 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
 		if (!x86_perf_event_set_period(event))
 			continue;
 
+		/*
+		 * For security, drop the skid kernel samples.
+		 */
+		if (skid_kernel_samples(event, regs))
+			continue;
+
 		if (perf_event_overflow(event, &data, regs))
 			x86_pmu_stop(event, 0);
 	}
@@ -1679,6 +1685,24 @@ ssize_t events_ht_sysfs_show(struct device *dev, struct device_attribute *attr,
 			pmu_attr->event_str_noht);
 }
 
+bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs)
+{
+	u64 ip;
+
+	/*
+	 * Without PEBS, we may get kernel samples even though
+	 * exclude_kernel is specified due to skid in sampling.
+	 */
+	if ((event->attr.exclude_kernel) &&
+	    (event->attr.sample_type & PERF_SAMPLE_IP)) {
+		ip = perf_instruction_pointer(regs);
+		if (kernel_ip(ip))
+			return true;
+	}
+
+	return false;
+}
+
 EVENT_ATTR(cpu-cycles,			CPU_CYCLES		);
 EVENT_ATTR(instructions,		INSTRUCTIONS		);
 EVENT_ATTR(cache-references,		CACHE_REFERENCES	);
diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index a6d91d4..8e9c9e8 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -2193,6 +2193,12 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
 		if (has_branch_stack(event))
 			data.br_stack = &cpuc->lbr_stack;
 
+		/*
+		 * For security, drop the skid kernel samples.
+		 */
+		if (skid_kernel_samples(event, regs))
+			continue;
+
 		if (perf_event_overflow(event, &data, regs))
 			x86_pmu_stop(event, 0);
 	}
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index be3d362..73fe023 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -818,6 +818,8 @@ ssize_t events_sysfs_show(struct device *dev, struct device_attribute *attr,
 ssize_t events_ht_sysfs_show(struct device *dev, struct device_attribute *attr,
 			  char *page);
 
+bool skid_kernel_samples(struct perf_event *event, struct pt_regs *regs);
+
 #ifdef CONFIG_CPU_SUP_AMD
 
 int amd_pmu_init(void);
-- 
2.7.4

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin, Yao]

SNIP

> I would much rather see this in generic code, somewhere around
> __perf_event_overflow() I suppose. That would retain proper accounting
> for the interrupt rate etc..
>
> Also it would work for all architectures. Because I'm thinking more than
> just x86 will suffer from skid.
Yes, moving to generic code is better.  Thanks for the suggestion! I 
will do that.

> If you're really worried, I suppose you can put it behind a PERF_PMU_CAP
> flag or something.
I guess what you are suggesting is to add checking like:

if (is_sampling_event(event)) {
     if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
         return;
     }
}

Is my understanding correct?

Thanks
Jin Yao

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Peter Zijlstra]

On Fri, May 19, 2017 at 08:06:09PM +0800, Jin, Yao wrote:
> SNIP
> 
> > I would much rather see this in generic code, somewhere around
> > __perf_event_overflow() I suppose. That would retain proper accounting
> > for the interrupt rate etc..
> > 
> > Also it would work for all architectures. Because I'm thinking more than
> > just x86 will suffer from skid.
> Yes, moving to generic code is better.  Thanks for the suggestion! I will do
> that.
> 
> > If you're really worried, I suppose you can put it behind a PERF_PMU_CAP
> > flag or something.
> I guess what you are suggesting is to add checking like:
> 
> if (is_sampling_event(event)) {
>     if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
>         return;
>     }
> }

Ah, I was more thinking of something like PERF_PMU_CAP_NO_SKID or
something that would skip the test and preserve current behaviour.

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin, Yao]

On 5/19/2017 8:10 PM, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 08:06:09PM +0800, Jin, Yao wrote:
>> SNIP
>>
>>> I would much rather see this in generic code, somewhere around
>>> __perf_event_overflow() I suppose. That would retain proper accounting
>>> for the interrupt rate etc..
>>>
>>> Also it would work for all architectures. Because I'm thinking more than
>>> just x86 will suffer from skid.
>> Yes, moving to generic code is better.  Thanks for the suggestion! I will do
>> that.
>>
>>> If you're really worried, I suppose you can put it behind a PERF_PMU_CAP
>>> flag or something.
>> I guess what you are suggesting is to add checking like:
>>
>> if (is_sampling_event(event)) {
>>      if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
>>          return;
>>      }
>> }
> Ah, I was more thinking of something like PERF_PMU_CAP_NO_SKID or
> something that would skip the test and preserve current behaviour.

OK, I understand now. For example, for PEBS event, its capabilities 
should be set with PERF_PMU_CAP_NO_SKID.

If the event's capabilities is set with PERF_PMU_CAP_NO_SKID, it should 
skip the checking and keep current behavior.

Thanks
Jin Yao

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Peter Zijlstra]

On Fri, May 19, 2017 at 08:24:19PM +0800, Jin, Yao wrote:
> > Ah, I was more thinking of something like PERF_PMU_CAP_NO_SKID or
> > something that would skip the test and preserve current behaviour.
> 
> OK, I understand now. For example, for PEBS event, its capabilities should
> be set with PERF_PMU_CAP_NO_SKID.

Except you cannot in fact do that, since PEBS is the same struct pmu as
the normal counters (they share counter space after all).

Also, weren't there PEBS errata that would allow this to happen?

But no, more for other architectures to opt out for some reason. But I'm
thinking we want to start out by unconditionally doing this. It would be
good to try and Cc most arch pmu maintainers on this though, so they can
object.

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin, Yao]

On 5/19/2017 8:36 PM, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 08:24:19PM +0800, Jin, Yao wrote:
>>> Ah, I was more thinking of something like PERF_PMU_CAP_NO_SKID or
>>> something that would skip the test and preserve current behaviour.
>> OK, I understand now. For example, for PEBS event, its capabilities should
>> be set with PERF_PMU_CAP_NO_SKID.
> Except you cannot in fact do that, since PEBS is the same struct pmu as
> the normal counters (they share counter space after all).
>
> Also, weren't there PEBS errata that would allow this to happen?
>
> But no, more for other architectures to opt out for some reason. But I'm
> thinking we want to start out by unconditionally doing this. It would be
> good to try and Cc most arch pmu maintainers on this though, so they can
> object.
>
I'm thinking v2 of patch will only do simple tasks:

1. Define PERF_PMU_CAP_NO_SKID but don't bind it to any event.

2. Move the skid checking from x86 specific code to generic code. Before 
performing skid checking, test the PERF_PMU_CAP_NO_SKID bit first.

For binding PERF_PMU_CAP_NO_SKID to event, that may be other arch 
related patches.

Thanks
Jin Yao

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin, Yao]

On 5/19/2017 9:33 PM, Jin, Yao wrote:
>
>
> On 5/19/2017 8:36 PM, Peter Zijlstra wrote:
>> On Fri, May 19, 2017 at 08:24:19PM +0800, Jin, Yao wrote:
>>>> Ah, I was more thinking of something like PERF_PMU_CAP_NO_SKID or
>>>> something that would skip the test and preserve current behaviour.
>>> OK, I understand now. For example, for PEBS event, its capabilities 
>>> should
>>> be set with PERF_PMU_CAP_NO_SKID.
>> Except you cannot in fact do that, since PEBS is the same struct pmu as
>> the normal counters (they share counter space after all).
>>
>> Also, weren't there PEBS errata that would allow this to happen?
>>
>> But no, more for other architectures to opt out for some reason. But I'm
>> thinking we want to start out by unconditionally doing this. It would be
>> good to try and Cc most arch pmu maintainers on this though, so they can
>> object.
>>
> I'm thinking v2 of patch will only do simple tasks:
>
> 1. Define PERF_PMU_CAP_NO_SKID but don't bind it to any event.
>
> 2. Move the skid checking from x86 specific code to generic code. 
> Before performing skid checking, test the PERF_PMU_CAP_NO_SKID bit first.
>
> For binding PERF_PMU_CAP_NO_SKID to event, that may be other arch 
> related patches.
>
> Thanks
> Jin Yao
>
>
Hi Peter,

Maybe it's not very easy to move the skid checking to generic code 
because we don't have a common kernel_ip() available to determine if ip 
is a kernel address.

I was trying to move kernel_ip() from arch/x86/events/perf_event.h to 
generic code, but some difficulties I have:

For example, in new kernel_ip(), we may use many conditional-compilation 
for all arch, for example:

#ifdef CONFIG_X86_32
     return ip > PAGE_OFFSET;
#endif

#ifdef CONFIG_X86_64
     return (long)ip < 0;
#endif

#ifdef CONFIG_ARM....
......
#ifdef CONFIG_MIPS....
......

But the code is being ugly and hard to maintain. And frankly I don't 
know kernel address space for all arch.

Any idea? Could we just do at x86 side this time?

Thanks
Jin Yao

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Mark Rutland]

On Mon, May 22, 2017 at 10:12:22AM +0800, Jin, Yao wrote:
> Hi Peter,
> 
> Maybe it's not very easy to move the skid checking to generic code
> because we don't have a common kernel_ip() available to determine if
> ip is a kernel address.
> 
> I was trying to move kernel_ip() from arch/x86/events/perf_event.h
> to generic code, but some difficulties I have:
> 
> For example, in new kernel_ip(), we may use many
> conditional-compilation for all arch, for example:
> 
> #ifdef CONFIG_X86_32
>     return ip > PAGE_OFFSET;
> #endif
> 
> #ifdef CONFIG_X86_64
>     return (long)ip < 0;
> #endif
> 
> #ifdef CONFIG_ARM....
> ......
> #ifdef CONFIG_MIPS....
> ......
> 
> But the code is being ugly and hard to maintain. And frankly I don't
> know kernel address space for all arch.
> 
> Any idea? Could we just do at x86 side this time?

Can we not check user_mode(regs) for all architectures?

!user_mode(regs) implies a kernel sample.

Thanks,
Mark.

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Peter Zijlstra]

On Mon, May 22, 2017 at 09:45:30AM +0100, Mark Rutland wrote:
> On Mon, May 22, 2017 at 10:12:22AM +0800, Jin, Yao wrote:

> > But the code is being ugly and hard to maintain. And frankly I don't
> > know kernel address space for all arch.
> > 
> > Any idea? Could we just do at x86 side this time?
> 
> Can we not check user_mode(regs) for all architectures?
> 
> !user_mode(regs) implies a kernel sample.

Yes, that should work at that point. We specifically already rely on
user_mode() in the generic code.

On x86 we specifically set regs->cs to match regs->ip (in cases where
this isn't necessarily so) before calling into the generic code to make
this work.

Re: [PATCH] perf/x86/intel: Drop kernel samples even though :u is specified

[Jin, Yao]

On 5/22/2017 5:26 PM, Peter Zijlstra wrote:
> On Mon, May 22, 2017 at 09:45:30AM +0100, Mark Rutland wrote:
>> On Mon, May 22, 2017 at 10:12:22AM +0800, Jin, Yao wrote:
>>> But the code is being ugly and hard to maintain. And frankly I don't
>>> know kernel address space for all arch.
>>>
>>> Any idea? Could we just do at x86 side this time?
>> Can we not check user_mode(regs) for all architectures?
>>
>> !user_mode(regs) implies a kernel sample.
> Yes, that should work at that point. We specifically already rely on
> user_mode() in the generic code.
>
> On x86 we specifically set regs->cs to match regs->ip (in cases where
> this isn't necessarily so) before calling into the generic code to make
> this work.
>
Got it.
Thanks Mark, thanks Peter!