From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932281AbcHOIGx (ORCPT ); Mon, 15 Aug 2016 04:06:53 -0400 Received: from wtarreau.pck.nerim.net ([62.212.114.60]:40141 "EHLO 1wt.eu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932249AbcHOIGt (ORCPT ); Mon, 15 Aug 2016 04:06:49 -0400 Date: Mon, 15 Aug 2016 10:06:41 +0200 From: Willy Tarreau To: Vegard Nossum Cc: Al Viro , linux-kernel@vger.kernel.org Subject: Re: [PATCH] fs/pipe: fix shift by 64 in F_SETPIPE_SZ Message-ID: <20160815080641.GA14695@1wt.eu> References: <1471005340-13682-1-git-send-email-vegard.nossum@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1471005340-13682-1-git-send-email-vegard.nossum@oracle.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Fri, Aug 12, 2016 at 02:35:40PM +0200, Vegard Nossum wrote: > 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). Why wouldn't we limit it to LONG_MAX and change round_pipe_size() to take an unsigned long in argument instead ? On 64-bit it would allow more than 2GB (even if I really doubt anybody will ever need this). Also, strictly speaking in your case it's not INT_MAX which is the absolute limit but UINT_MAX - PAGE_SIZE since it's a round up issue before being a shift issue. But that's mostly a detail I guess. Overall I think your change is right. Regards, Willy