[perf] yet another 32/64-bit range check failure

[perf] yet another 32/64-bit range check failure

[Vince Weaver]

More fun found by the perf_fuzzer...

In kernel/events/core.c 
SYSCALL_DEFINE5(perf_event_open,

We check if flags is valid like this:

        /* for future expandability... */
        if (flags & ~PERF_FLAG_ALL)
                return -EINVAL;

but flags is a 64-bit value but ~PERF_FLAG_ALL is 32-bit.

This means values like 0x800000000000ULL are treated as valid even though 
they aren't.

This is allowing events to be allocated memory but not being freed somehow
before returning EINVAL (a memory leak).
At least it looks like this is happening in the huge traces I have trying 
to track down the perf_fuzzer memory corruption bug.

I'd send a patch to fix the above, but it's late and I can't figure out 
where exactly to stick ULL to get PERF_FLAG_ALL to be upgraded to 64-bit.

Vince

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Tue, 22 Apr 2014, Vince Weaver wrote:

> This is allowing events to be allocated memory but not being freed somehow
> before returning EINVAL (a memory leak).
> At least it looks like this is happening in the huge traces I have trying 
> to track down the perf_fuzzer memory corruption bug.

I can't find where the memory leak happens, but it looks like this in the 
trace:

[ 3524.626452] perf_fuz-1798    0.... 1271584315us : sys_enter: NR 298 (698e40, 706, ffffffff, f, 800000000000, 800000000000)
[ 3524.642312] perf_fuz-1798    0.... 1271584324us : kmalloc: call_site=ffffffff8113a575 ptr=ffff88007d5b0800 bytes_req=1272 bytes_alloc=2048 gfp_flags=GFP_KERNEL|GFP_ZERO
[ 3524.662598] perf_fuz-1798    0.... 1271584337us : sys_exit: NR 298 = -22

The call site for the kmalloc is in perf_event_alloc()

The memory is eventually freed as:

[ 3547.895534]   <idle>-0       0.Ns. 1271595088us : kfree: call_site=ffffffff811316aa ptr=ffff88007d5b0800


Vince

Re: [perf] yet another 32/64-bit range check failure

[Peter Zijlstra]

On Wed, Apr 23, 2014 at 12:14:52AM -0400, Vince Weaver wrote:
> On Tue, 22 Apr 2014, Vince Weaver wrote:
> 
> > This is allowing events to be allocated memory but not being freed somehow
> > before returning EINVAL (a memory leak).
> > At least it looks like this is happening in the huge traces I have trying 
> > to track down the perf_fuzzer memory corruption bug.
> 
> I can't find where the memory leak happens, but it looks like this in the 
> trace:
> 
> [ 3524.626452] perf_fuz-1798    0.... 1271584315us : sys_enter: NR 298 (698e40, 706, ffffffff, f, 800000000000, 800000000000)
> [ 3524.642312] perf_fuz-1798    0.... 1271584324us : kmalloc: call_site=ffffffff8113a575 ptr=ffff88007d5b0800 bytes_req=1272 bytes_alloc=2048 gfp_flags=GFP_KERNEL|GFP_ZERO
> [ 3524.662598] perf_fuz-1798    0.... 1271584337us : sys_exit: NR 298 = -22
> 
> The call site for the kmalloc is in perf_event_alloc()
> 
> The memory is eventually freed as:
> 
> [ 3547.895534]   <idle>-0       0.Ns. 1271595088us : kfree: call_site=ffffffff811316aa ptr=ffff88007d5b0800

So perf_event_open() -> err_alloc: -> free_event() -> __free_event() ->
call_rcu() -> free_event_rcu() -> kfree().

Would explain that, right? The memory is RCU freed, which means we need
to wait a grace period before releasing it.

Re: [perf] yet another 32/64-bit range check failure

[Peter Zijlstra]

On Tue, Apr 22, 2014 at 11:40:07PM -0400, Vince Weaver wrote:
> 
> More fun found by the perf_fuzzer...
> 
> In kernel/events/core.c 
> SYSCALL_DEFINE5(perf_event_open,
> 
> We check if flags is valid like this:
> 
>         /* for future expandability... */
>         if (flags & ~PERF_FLAG_ALL)
>                 return -EINVAL;
> 
> but flags is a 64-bit value but ~PERF_FLAG_ALL is 32-bit.
> 
> This means values like 0x800000000000ULL are treated as valid even though 
> they aren't.
> 
> This is allowing events to be allocated memory but not being freed somehow
> before returning EINVAL (a memory leak).
> At least it looks like this is happening in the huge traces I have trying 
> to track down the perf_fuzzer memory corruption bug.
> 
> I'd send a patch to fix the above, but it's late and I can't figure out 
> where exactly to stick ULL to get PERF_FLAG_ALL to be upgraded to 64-bit.
> 
> Vince

Something like so should do I suppose.

---
Subject: perf: Fix perf_event_open(.flags) test

Vince noticed that we test the (unsigned long) flags field against an
(unsigned int) constant. This would allow setting the high bits on 64bit
platforms and not get an error.

There is nothing that uses the high bits, so it should be entirely
harmless, but we don't want userspace to accidentally set them anyway,
so fix the constants.

Reported-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 include/uapi/linux/perf_event.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 853bc1ccb395..e3fc8f09d110 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -722,10 +722,10 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
-#define PERF_FLAG_FD_NO_GROUP		(1U << 0)
-#define PERF_FLAG_FD_OUTPUT		(1U << 1)
-#define PERF_FLAG_PID_CGROUP		(1U << 2) /* pid=cgroup id, per-cpu mode only */
-#define PERF_FLAG_FD_CLOEXEC		(1U << 3) /* O_CLOEXEC */
+#define PERF_FLAG_FD_NO_GROUP		(1UL << 0)
+#define PERF_FLAG_FD_OUTPUT		(1UL << 1)
+#define PERF_FLAG_PID_CGROUP		(1UL << 2) /* pid=cgroup id, per-cpu mode only */
+#define PERF_FLAG_FD_CLOEXEC		(1UL << 3) /* O_CLOEXEC */
 
 union perf_mem_data_src {
 	__u64 val;

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Wed, 23 Apr 2014, Peter Zijlstra wrote:
> 
> So perf_event_open() -> err_alloc: -> free_event() -> __free_event() ->
> call_rcu() -> free_event_rcu() -> kfree().
> 
> Would explain that, right? The memory is RCU freed, which means we need
> to wait a grace period before releasing it.

ah yes, RCU.  That does make debugging this issue a lot harder.

Back to trying to get the bug to trigger in a useful location while ftrace 
is running.  I keep triggering it immediately after the compiler generates 
helpful code like
	mov    0x40(%rbx),%rbx
so the address is lost and the register dump just holds 0x6b6b6b6b6b6b6b6b.

Vince

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Wed, 23 Apr 2014, Peter Zijlstra wrote:

> On Tue, Apr 22, 2014 at 11:40:07PM -0400, Vince Weaver wrote:
> > 
> > More fun found by the perf_fuzzer...
> > 
> > In kernel/events/core.c 
> > SYSCALL_DEFINE5(perf_event_open,
> > 
> > We check if flags is valid like this:
> > 
> >         /* for future expandability... */
> >         if (flags & ~PERF_FLAG_ALL)
> >                 return -EINVAL;
> > 
> > but flags is a 64-bit value but ~PERF_FLAG_ALL is 32-bit.
> > 
> > This means values like 0x800000000000ULL are treated as valid even though 
> > they aren't.
> > 
> > This is allowing events to be allocated memory but not being freed somehow
> > before returning EINVAL (a memory leak).
> > At least it looks like this is happening in the huge traces I have trying 
> > to track down the perf_fuzzer memory corruption bug.
> > 
> > I'd send a patch to fix the above, but it's late and I can't figure out 
> > where exactly to stick ULL to get PERF_FLAG_ALL to be upgraded to 64-bit.
> > 
> > Vince
> 
> Something like so should do I suppose.
> 
> ---
> Subject: perf: Fix perf_event_open(.flags) test
> 
> Vince noticed that we test the (unsigned long) flags field against an
> (unsigned int) constant. This would allow setting the high bits on 64bit
> platforms and not get an error.
> 
> There is nothing that uses the high bits, so it should be entirely
> harmless, but we don't want userspace to accidentally set them anyway,
> so fix the constants.

I suppose I should make a patch for attr->sample_type and 
attr->read_format which after a quick audit seem to exhibit the same 
problem?

Vince

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Wed, 23 Apr 2014, Vince Weaver wrote:
> 
> I suppose I should make a patch for attr->sample_type and 
> attr->read_format which after a quick audit seem to exhibit the same 
> problem?

nevermind, PERF_FORMAT_MAX and PERF_SAMPLE_MAX are enum values, not 
#defines, so they handle being 64-bit properly.

Vince

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Wed, 23 Apr 2014, Peter Zijlstra wrote:
> 
> Something like so should do I suppose.
> 
> ---
> Subject: perf: Fix perf_event_open(.flags) test
> 
> Vince noticed that we test the (unsigned long) flags field against an
> (unsigned int) constant. This would allow setting the high bits on 64bit
> platforms and not get an error.
> 
> There is nothing that uses the high bits, so it should be entirely
> harmless, but we don't want userspace to accidentally set them anyway,
> so fix the constants.
> 
> Reported-by: Vince Weaver <vincent.weaver@maine.edu>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>

Tested-by: Vince Weaver <vincent.weaver@maine.edu>

Your patch fixes the problem, or at least the test I wrote to check the 
issue now fails properly.

Oddly, with this patch applied, it's made it a lot harder (but not 
impossible) to trigger the memory corruption bug, although that might just 
be coincidence.

Vince

Re: [perf] yet another 32/64-bit range check failure

[Vince Weaver]

On Thu, 24 Apr 2014, Vince Weaver wrote:

> On Wed, 23 Apr 2014, Peter Zijlstra wrote:
> > 
> > Something like so should do I suppose.
> > 
> > ---
> > Subject: perf: Fix perf_event_open(.flags) test
> > 
> > Vince noticed that we test the (unsigned long) flags field against an
> > (unsigned int) constant. This would allow setting the high bits on 64bit
> > platforms and not get an error.
> > 
> > There is nothing that uses the high bits, so it should be entirely
> > harmless, but we don't want userspace to accidentally set them anyway,
> > so fix the constants.
> > 
> > Reported-by: Vince Weaver <vincent.weaver@maine.edu>
> > Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> 
> Tested-by: Vince Weaver <vincent.weaver@maine.edu>
> 
> Your patch fixes the problem, or at least the test I wrote to check the 
> issue now fails properly.

Even though this isn't as pressing an issue as the other perf_event 
problems, I wanted to make sure this patch didn't get forgotten...

Vince

Re: [perf] yet another 32/64-bit range check failure

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

On Thu, May 15, 2014 at 01:07:34AM -0400, Vince Weaver wrote:
> On Thu, 24 Apr 2014, Vince Weaver wrote:
> 
> > On Wed, 23 Apr 2014, Peter Zijlstra wrote:
> > > 
> > > Something like so should do I suppose.
> > > 
> > > ---
> > > Subject: perf: Fix perf_event_open(.flags) test
> > > 
> > > Vince noticed that we test the (unsigned long) flags field against an
> > > (unsigned int) constant. This would allow setting the high bits on 64bit
> > > platforms and not get an error.
> > > 
> > > There is nothing that uses the high bits, so it should be entirely
> > > harmless, but we don't want userspace to accidentally set them anyway,
> > > so fix the constants.
> > > 
> > > Reported-by: Vince Weaver <vincent.weaver@maine.edu>
> > > Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> > 
> > Tested-by: Vince Weaver <vincent.weaver@maine.edu>
> > 
> > Your patch fixes the problem, or at least the test I wrote to check the 
> > issue now fails properly.
> 
> Even though this isn't as pressing an issue as the other perf_event 
> problems, I wanted to make sure this patch didn't get forgotten...

Thanks for reminding me, got it queued now.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[tip:perf/core] perf: Fix perf_event_open(.flags) test

[tip-bot for Peter Zijlstra]

Commit-ID:  643fd0b9f5dc40fedbfbb908ebe6f1169284f7d8
Gitweb:     http://git.kernel.org/tip/643fd0b9f5dc40fedbfbb908ebe6f1169284f7d8
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 23 Apr 2014 12:22:54 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 19 May 2014 21:52:59 +0900

perf: Fix perf_event_open(.flags) test

Vince noticed that we test the (unsigned long) flags field against an
(unsigned int) constant. This would allow setting the high bits on 64bit
platforms and not get an error.

There is nothing that uses the high bits, so it should be entirely
harmless, but we don't want userspace to accidentally set them anyway,
so fix the constants.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Reported-by: Vince Weaver <vincent.weaver@maine.edu>
Tested-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20140423102254.GL11096@twins.programming.kicks-ass.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/uapi/linux/perf_event.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
index 853bc1c..e3fc8f0 100644
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -722,10 +722,10 @@ enum perf_callchain_context {
 	PERF_CONTEXT_MAX		= (__u64)-4095,
 };
 
-#define PERF_FLAG_FD_NO_GROUP		(1U << 0)
-#define PERF_FLAG_FD_OUTPUT		(1U << 1)
-#define PERF_FLAG_PID_CGROUP		(1U << 2) /* pid=cgroup id, per-cpu mode only */
-#define PERF_FLAG_FD_CLOEXEC		(1U << 3) /* O_CLOEXEC */
+#define PERF_FLAG_FD_NO_GROUP		(1UL << 0)
+#define PERF_FLAG_FD_OUTPUT		(1UL << 1)
+#define PERF_FLAG_PID_CGROUP		(1UL << 2) /* pid=cgroup id, per-cpu mode only */
+#define PERF_FLAG_FD_CLOEXEC		(1UL << 3) /* O_CLOEXEC */
 
 union perf_mem_data_src {
 	__u64 val;