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

DISCUSSION Sasha Levin 4 participants 6 messages Jan 10, 2016 – Mar 21, 2016 perf events

lore

Share & tools

lore

Sasha Levin

author

2016-01-10 20:55 UTC #0 lore raw

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

Peter Zijlstra

maintainer

2016-01-19 14:31 UTC #1 lore raw

Quoted 7 lines · 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

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.

Sasha Levin

author

2016-01-21 23:35 UTC #2 lore raw

Quoted 6 lines · Peter Zijlstra

Quoted 5 lines · Sasha Levin
Hi all,
While fuzzing with trinity inside a KVM tools guest, running the latest -next kernel, I've hit the following warning:

Quoted 8 lines

Quoted 2 lines
[ 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:

kernel/events/internal.h +1 -0

index 2bbad9c..f627a40 100644
@@ -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  */

kernel/events/ring_buffer.c +2 -0

index adfdc05..65346f8 100644
@@ -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

Andrey Ryabinin

2016-01-22 12:49 UTC #3 lore raw

In reply to Peter Zijlstra

2016-01-19 17:31 GMT+03:00 Peter Zijlstra <[email protected]>:

Quoted 11 lines · Sasha Levin

Quoted 7 lines
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);

Peter Zijlstra

maintainer

2016-01-29 14:18 UTC #4 lore raw

Quoted 7 lines · Andrey Ryabinin

2016-01-19 17:31 GMT+03:00 Peter Zijlstra <[email protected]>:
Quoted 6 lines · Sasha Levin
Quoted 5 lines
Hi all,
While fuzzing with trinity inside a KVM tools guest, running the latest -next kernel, I've hit the following warning:

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
[ 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(-)

kernel/events/ring_buffer.c +4 -2

index adfdc0536117..345130705c49 100644
@@ -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-bot for Peter Zijlstra

merge bot

2016-03-21 09:55 UTC #5 lore raw

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

perf/core: Fix Undefined behaviour in rb_alloc()

Commit-ID:  8184059e93c200757f5c0805dae0f14e880eab5d
Gitweb:     http://git.kernel.org/tip/8184059e93c200757f5c0805dae0f14e880eab5d
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Fri, 29 Jan 2016 15:17:51 +0100
Committer:  Ingo Molnar <[email protected]>
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 <[email protected]>

Reported-by Andrey Ryabinin <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc David Ahern <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Vince Weaver <[email protected]>

Link http://lkml.kernel.org/r/20160129141751.GA407@worktop open here

Signed-off-by Ingo Molnar <[email protected]>

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

kernel/events/ring_buffer.c +4 -2

index 1faad2c..c61f0cb 100644
@@ -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);