From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752614AbcHLMgN (ORCPT ); Fri, 12 Aug 2016 08:36:13 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:19041 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752233AbcHLMgM (ORCPT ); Fri, 12 Aug 2016 08:36:12 -0400 From: Vegard Nossum To: Al Viro Cc: linux-kernel@vger.kernel.org, Vegard Nossum , Willy Tarreau Subject: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Date: Fri, 12 Aug 2016 14:35:40 +0200 Message-Id: <1471005340-13682-1-git-send-email-vegard.nossum@oracle.com> X-Mailer: git-send-email 1.9.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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: [] dump_stack+0xac/0xfc [] ubsan_epilogue+0xd/0x8a [] __ubsan_handle_shift_out_of_bounds+0x255/0x29a [] pipe_fcntl+0x59b/0x800 [] SyS_fcntl+0x69a/0xe50 [] do_syscall_64+0x1b3/0x4b0 [] 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 Signed-off-by: Vegard Nossum --- 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