linux-kernel - [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 12 Aug 2016 14:35:40 +0200
From:	Vegard Nossum <vegard.nossum@...cle.com>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	linux-kernel@...r.kernel.org,
	Vegard Nossum <vegard.nossum@...cle.com>,
	Willy Tarreau <w@....eu>
Subject: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ

I got this:

    ================================================================================
    UBSAN: Undefined behaviour in ./include/linux/log2.h:63:13
    shift exponent 64 is too large for 64-bit type 'long unsigned int'
    CPU: 0 PID: 5351 Comm: trinity-c0 Not tainted 4.8.0-rc1+ #84
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
     0000000000000000 ffff880115c67c08 ffffffff82344f40 0000000041b58ab3
     ffffffff84f98000 ffffffff82344e94 ffff880115c67c30 ffff880115c67be0
     0000000000000001 ffff880115c679e8 dffffc0000000000 ffffffff85bf0820
    Call Trace:
     [<ffffffff82344f40>] dump_stack+0xac/0xfc
     [<ffffffff8242f5a8>] ubsan_epilogue+0xd/0x8a
     [<ffffffff82430c31>] __ubsan_handle_shift_out_of_bounds+0x255/0x29a
     [<ffffffff818229ab>] pipe_fcntl+0x59b/0x800
     [<ffffffff8184504a>] SyS_fcntl+0x69a/0xe50
     [<ffffffff81007bd3>] do_syscall_64+0x1b3/0x4b0
     [<ffffffff845f946a>] entry_SYSCALL64_slow_path+0x25/0x25
    ================================================================================

The problem is that if the argument (an unsigned long) passed to
F_SETPIPE_SZ is either 0 or greater than UINT_MAX, then
roundup_pow_of_two() will hit undefined behavior because the shift
width will be 64.

Even if we limited the argument to UINT_MAX, we would still need to
keep the !nr_pages check, as passing anything greater than INT_MAX
will give a nr_pages inside round_pipe_size() of (1 << 20) which
then gets truncated to 0 when we convert it to an unsigned int
(because (1 << 20) << PAGE_SHIFT == 1 << 32).

If we limit it to INT_MAX, then we know nr_pages will never be 0.
Rudimentary boundary analysis (both 32- and 64-bit):

  arg == 0: gets rejected with -EINVAL by our check
  arg == 1: round_pipe_size() rounds up to PAGE_SIZE and returns PAGE_SIZE
  arg == INT_MAX - 1: round_pipe_size() returns 0x80000000
  arg == INT_MAX: round_pipe_size() returns 0x80000000
  arg > INT_MAX: gets rejected with -EINVAL by our check

In practice the undefined behaviour causes my gcc at least to return
0 for the large shift (i.e. 1ULL << 64 == 0), so nothing bad happens
because this is caught by the if (!nr_pages) check. But I don't think
we can bank on this always being the case. This patch avoids the
undefined behaviour completely. (Stable not on Cc since it violates
the “no "This could be a problem"” rule.)

Tested on 32- and 64-bit x86/UML.

Cc: Willy Tarreau <w@....eu>
Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
---
 fs/pipe.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 4ebe6b2..42ea89f 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1115,13 +1115,13 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 	case F_SETPIPE_SZ: {
 		unsigned int size, nr_pages;
 
-		size = round_pipe_size(arg);
-		nr_pages = size >> PAGE_SHIFT;
-
 		ret = -EINVAL;
-		if (!nr_pages)
+		if (!arg || arg > INT_MAX)
 			goto out;
 
+		size = round_pipe_size(arg);
+		nr_pages = size >> PAGE_SHIFT;
+
 		if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
 			ret = -EPERM;
 			goto out;
-- 
1.9.1