perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest, running the latest -next
kernel, I've hit the following warning:

[ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
[ 3494.030647] shift exponent -1 is negative
[ 3494.031025] CPU: 1 PID: 28506 Comm: kworker/1:2 Not tainted 4.4.0-rc8-next-20160108-sasha-00024-gaaecb9a #2780
[ 3494.032861] Workqueue: events rb_free_work
[ 3494.033195]  1ffff10018bb2f1b 0000000025cc19e7 ffff8800c5d97958 ffffffffa101a182
[ 3494.033871]  0000000041b58ab3 ffffffffac1b3838 ffffffffa101a0b7 ffff8800c5d97920
[ 3494.034469]  ffffffffae01db80 ffff8800c5d97940 0000000025cc19e7 ffffffffae01db80
[ 3494.035097] Call Trace:
[ 3494.035336]  [<ffffffffa101a182>] dump_stack+0xcb/0x149
[ 3494.036171]  [<ffffffffa1114296>] ubsan_epilogue+0x12/0x8f
[ 3494.036568]  [<ffffffffa1114e9a>] __ubsan_handle_shift_out_of_bounds+0x2a3/0x308
[ 3494.038670]  [<ffffffff9f6274ee>] rb_free_work+0xae/0x1a0
[ 3494.039491]  [<ffffffff9f3ac74c>] process_one_work+0xb6c/0x13d0
[ 3494.042924]  [<ffffffff9f3adce3>] worker_thread+0xd33/0x1150
[ 3494.043923]  [<ffffffff9f3c4239>] kthread+0x2f9/0x310


Thanks,
Sasha

Re: perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

[Peter Zijlstra]

On Sun, Jan 10, 2016 at 03:55:13PM -0500, Sasha Levin wrote:
> Hi all,
> 
> While fuzzing with trinity inside a KVM tools guest, running the latest -next
> kernel, I've hit the following warning:
> 
> [ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
> [ 3494.030647] shift exponent -1 is negative

That's rb->page_order == -1, which should 'never' happen, curious!

Funny though that rb::page_order is the exact field _after_ rb::work, ho
humm.

Re: perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

[Sasha Levin]

On 01/19/2016 09:31 AM, Peter Zijlstra wrote:
> On Sun, Jan 10, 2016 at 03:55:13PM -0500, Sasha Levin wrote:
>> Hi all,
>>
>> While fuzzing with trinity inside a KVM tools guest, running the latest -next
>> kernel, I've hit the following warning:
>>
>> [ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
>> [ 3494.030647] shift exponent -1 is negative
> 
> That's rb->page_order == -1, which should 'never' happen, curious!
> 
> Funny though that rb::page_order is the exact field _after_ rb::work, ho
> humm.
> 

I've tested your theory using:

diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 2bbad9c..f627a40 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -14,6 +14,7 @@ struct ring_buffer {
        struct irq_work                 irq_work;
 #ifdef CONFIG_PERF_USE_VMALLOC
        struct work_struct              work;
+       unsigned long dummy;
        int                             page_order;     /* allocation order  */
 #endif
        int                             nr_pages;       /* nr of data pages  */
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index adfdc05..65346f8 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -682,6 +682,8 @@ void rb_free(struct ring_buffer *rb)
 #else
 static int data_page_nr(struct ring_buffer *rb)
 {
+       if (page_order(rb) < 0 || rb->dummy)
+               pr_emerg("*** %lx\n", rb->dummy);
        return rb->nr_pages << page_order(rb);
 }

But the output I'm seeing indicates that dummy isn't corrupted:

[  758.806091] *** 0
[  758.806821] ================================================================================
[  758.807961] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:687:22
[  758.808833] shift exponent -1 is negative
[...]


Thanks,
Sasha

Re: perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

[Andrey Ryabinin]

2016-01-19 17:31 GMT+03:00 Peter Zijlstra <peterz@infradead.org>:
> On Sun, Jan 10, 2016 at 03:55:13PM -0500, Sasha Levin wrote:
>> Hi all,
>>
>> While fuzzing with trinity inside a KVM tools guest, running the latest -next
>> kernel, I've hit the following warning:
>>
>> [ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
>> [ 3494.030647] shift exponent -1 is negative
>
> That's rb->page_order == -1, which should 'never' happen, curious!
>

It happens if nr_pages = 0:
     rb->page_order = ilog2(nr_pages);

Re: perf/ring-buffer: Undefined behaviour in kernel/events/ring_buffer.c:685:22

[Peter Zijlstra]

On Fri, Jan 22, 2016 at 03:48:55PM +0300, Andrey Ryabinin wrote:
> 2016-01-19 17:31 GMT+03:00 Peter Zijlstra <peterz@infradead.org>:
> > On Sun, Jan 10, 2016 at 03:55:13PM -0500, Sasha Levin wrote:
> >> Hi all,
> >>
> >> While fuzzing with trinity inside a KVM tools guest, running the latest -next
> >> kernel, I've hit the following warning:
> >>
> >> [ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
> >> [ 3494.030647] shift exponent -1 is negative
> >
> > That's rb->page_order == -1, which should 'never' happen, curious!
> >
> 
> It happens if nr_pages = 0:
>      rb->page_order = ilog2(nr_pages);

Ah indeed.

Something like so then. When !nr_pages, both variables should be 0 and
are due to the kzalloc() of rb slightly above.

---
 kernel/events/ring_buffer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index adfdc0536117..345130705c49 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -746,8 +746,10 @@ struct ring_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 
 	rb->user_page = all_buf;
 	rb->data_pages[0] = all_buf + PAGE_SIZE;
-	rb->page_order = ilog2(nr_pages);
-	rb->nr_pages = !!nr_pages;
+	if (nr_pages) {
+		rb->nr_pages = 1;
+		rb->page_order = ilog2(nr_pages);
+	}
 
 	ring_buffer_init(rb, watermark, flags);
 

[tip:perf/urgent] perf/core: Fix Undefined behaviour in rb_alloc()

[tip-bot for Peter Zijlstra]

Commit-ID:  8184059e93c200757f5c0805dae0f14e880eab5d
Gitweb:     http://git.kernel.org/tip/8184059e93c200757f5c0805dae0f14e880eab5d
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 29 Jan 2016 15:17:51 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 21 Mar 2016 09:08:18 +0100

perf/core: Fix Undefined behaviour in rb_alloc()

Sasha reported:

 [ 3494.030114] UBSAN: Undefined behaviour in kernel/events/ring_buffer.c:685:22
 [ 3494.030647] shift exponent -1 is negative

Andrey spotted that this is because:

  It happens if nr_pages = 0:
     rb->page_order = ilog2(nr_pages);

Fix it by making both assignments conditional on nr_pages; since
otherwise they should both be 0 anyway, and will be because of the
kzalloc() used to allocate the structure.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Reported-by: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Link: http://lkml.kernel.org/r/20160129141751.GA407@worktop
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/ring_buffer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 1faad2c..c61f0cb 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -746,8 +746,10 @@ struct ring_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
 
 	rb->user_page = all_buf;
 	rb->data_pages[0] = all_buf + PAGE_SIZE;
-	rb->page_order = ilog2(nr_pages);
-	rb->nr_pages = !!nr_pages;
+	if (nr_pages) {
+		rb->nr_pages = 1;
+		rb->page_order = ilog2(nr_pages);
+	}
 
 	ring_buffer_init(rb, watermark, flags);