[PATCH] pipe: handle zero sized F_SETPIPE_SZ fcntl

[PATCH] pipe: handle zero sized F_SETPIPE_SZ fcntl

[Nicolai Stange]

Syzkaller reported this

  UBSAN: Undefined behaviour in include/linux/log2.h:63:13
  shift exponent 64 is too large for 64-bit type 'long unsigned int'
  Call Trace:
   __dump_stack lib/dump_stack.c:15 [inline]
   dump_stack+0x108/0x19b lib/dump_stack.c:51
   ubsan_epilogue+0x12/0x8f lib/ubsan.c:164
   __ubsan_handle_shift_out_of_bounds+0x29c/0x300 lib/ubsan.c:421
   __roundup_pow_of_two include/linux/log2.h:63 [inline]
   round_pipe_size fs/pipe.c:1049 [inline]
   pipe_set_size fs/pipe.c:1042 [inline]
   pipe_fcntl+0x12a/0x6e0 fs/pipe.c:1159
   do_fcntl fs/fcntl.c:332 [inline]
   SYSC_fcntl fs/fcntl.c:372 [inline]
   SyS_fcntl+0xa90/0xb80 fs/fcntl.c:380
   entry_SYSCALL_64_fastpath+0x1f/0xc2

after it had tried to do this:

  fcntl(fd, F_SETPIPE_SZ, 0);

The reason is that round_pipe_size() rounds the requested size towards the
next multiple of PAGE_SIZE and passes the thus resulting number of pages
to roundup_pow_of_two(). Now, roundup_pow_of_two(0) is defined to be
undefined. More specifically,

  roundup_pow_of_two(0) == 1UL << fls_long(-1) == 1UL << BITS_PER_LONG

which evaluates to 1 on x86 (good), but to zero on ARMv7 (not good), for
example.

Whatever the resulting value is, pipe_set_size() will hand it to kcalloc(),
so it should better be nonzero.

Make round_pipe_size() handle the size == 0 case explicitly and let it
return PAGE_SIZE then.

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/pipe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index 73b84baf58f8..2c88fa8cbe3b 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1024,6 +1024,9 @@ static inline unsigned int round_pipe_size(unsigned int size)
 {
 	unsigned long nr_pages;
 
+	if (!size)
+		return PAGE_SIZE;
+
 	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
 }
-- 
2.11.1

Re: [PATCH] pipe: handle zero sized F_SETPIPE_SZ fcntl

[Christoph Hellwig]

Looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>