Subject: Re: UBSAN: Undefined error in log2.h
To: Shankara Pailoor <sp3485@columbia.edu>, linux-kernel@vger.kernel.org
References: <CAASgV=v8yk-d_MxTbOw8omrqgUK1SP88-dnng9EfzorzE+8hdg@mail.gmail.com>
Cc: Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>
From: Randy Dunlap <rdunlap@infradead.org>
Message-ID: <7ccba84e-7a41-3a89-ae01-fca62011081b@infradead.org>
Date: Wed, 6 Sep 2017 13:44:48 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <CAASgV=v8yk-d_MxTbOw8omrqgUK1SP88-dnng9EfzorzE+8hdg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3377
Lines: 111

[add Al Viro and linux-fsdevel]

On 09/05/17 21:58, Shankara Pailoor wrote:
> Hi,
> 
> I am hitting this bug when running the syzkaller fuzzer on kernel 4.13-rc7
> 
> Syzkaller hit 'UBSAN: Undefined behaviour in ./include/linux/log2.h:LINE' bug.
> 
> Guilty file: fs/pipe.c
> 
> Maintainers: []
> 
> UBSAN: Undefined behaviour in ./include/linux/log2.h:57:13
> shift exponent 64 is too large for 64-bit type 'long unsigned int'

See below.

> CPU: 3 PID: 2712 Comm: syz-executor0 Not tainted 4.13.0-rc7 #2
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> Call Trace:
>  __dump_stack lib/dump_stack.c:16 [inline]
>  dump_stack+0xf7/0x1ae lib/dump_stack.c:52
>  ubsan_epilogue+0x12/0x8f lib/ubsan.c:164
>  __ubsan_handle_shift_out_of_bounds+0x2b2/0x32c lib/ubsan.c:421
>  __roundup_pow_of_two include/linux/log2.h:57 [inline]
>  round_pipe_size fs/pipe.c:1027 [inline]
>  pipe_set_size fs/pipe.c:1041 [inline]
>  pipe_fcntl+0x6b6/0x7b0 fs/pipe.c:1158
>  do_fcntl+0x362/0x1250 fs/fcntl.c:416
>  SYSC_fcntl fs/fcntl.c:462 [inline]
>  SyS_fcntl+0xd6/0x110 fs/fcntl.c:447
>  entry_SYSCALL_64_fastpath+0x18/0xad
> RIP: 0033:0x451e59
> RSP: 002b:00007ffe1469d358 EFLAGS: 00000212 ORIG_RAX: 0000000000000048
> RAX: ffffffffffffffda RBX: 0000000000718000 RCX: 0000000000451e59
> RDX: 0000000000000000 RSI: 0000000000000407 RDI: 0000000000000003
> RBP: 0000000000000046 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000212 R12: 0000000020011ff8
> R13: 0000000000718000 R14: 0000000000000000 R15: 0000000000000000
> ================================================================================
> 
> I have the full reproducer program here: https://pastebin.com/mLZx0ySS
> 
> The main code is below:
> 
> long r[4];
> void loop()
> {
>     syscall(SYS_write, 1, "executing program\n", strlen("executing program\n"));
>     memset(r, -1, sizeof(r));
>     r[0] = syscall(__NR_mmap, 0x20000000ul, 0x12000ul, 0x3ul, 0x32ul,
> 0xfffffffffffffffful, 0x0ul);
>     r[1] = syscall(__NR_pipe, 0x20011ff8ul);
>     if (r[1] != -1)
>         NONFAILING(r[2] = *(uint32_t*)0x20011ff8);
>     r[3] = syscall(__NR_fcntl, r[2], 0x407ul, 0x0ul);

That's                                 F_SETPIPE_SZ to 0, right?

> }

so pipe_set_size(pipe, 0UL) does

	size = round_pipe_size(arg);
which is:

static inline unsigned int round_pipe_size(unsigned int size)
{
	unsigned long nr_pages;

	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
}

so we have:
	nr_pages = (0 + 4096 - 1) >> 12;
i.e.	nr_pages = 0

	return roundup_pow_of_two(0) << 12;
...	return __roundup_pow_of_two(0) << 12;

Look at __roundup_pow_of_two(0):
	return 1UL << fls_long(n - 1);
...	return 1UL << fls_long(-1)
...	return 1UL << 64;		// UBSAN kicks in.


We could try to fix the basic low-level functions to handle 0 (where <linux/log2.h>
says the result is undefined when n == 0), but ISTM the safest thing to do for now
would be just to patch fs/pipe.c as follows;

--- lnx-413.orig/fs/pipe.c
+++ lnx-413/fs/pipe.c
@@ -1038,6 +1038,8 @@ static long pipe_set_size(struct pipe_in
 	unsigned long user_bufs;
 	long ret = 0;
 
+	if (!arg)
+		arg = PAGE_SIZE;
 	size = round_pipe_size(arg);
 	nr_pages = size >> PAGE_SHIFT;
 

I think that I have stared at this long enough for now.

eh?
-- 
~Randy