From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8273C4363A for ; Thu, 22 Oct 2020 17:49:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7B97E208B3 for ; Thu, 22 Oct 2020 17:49:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603388944; bh=3M4q1AUzE86IrFjVI2Fx8qvLy0zAJTNJFVgWfdIdoS4=; h=Subject:From:To:Cc:Date:In-Reply-To:References:List-ID:From; b=T0umscr7ZvpBLH+rrf3JKzEYaOPsIKh5DLGuP7pab92mWYCyFgmFUZaRRc01HzLDn It8/S40xplo6uvFMTA8mRYJenX3JWPSSqeQnq+36od0ogPCxU1c1kD4tHVmMmHQnkr TQPkoxgnfDGcA+9nqMAR99a7oiCwtxTqQw70KP0A= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2900993AbgJVRtE (ORCPT ); Thu, 22 Oct 2020 13:49:04 -0400 Received: from mail.kernel.org ([198.145.29.99]:52428 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2900943AbgJVRtD (ORCPT ); Thu, 22 Oct 2020 13:49:03 -0400 Received: from tleilax.poochiereds.net (68-20-15-154.lightspeed.rlghnc.sbcglobal.net [68.20.15.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 58989205CA; Thu, 22 Oct 2020 17:49:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603388943; bh=3M4q1AUzE86IrFjVI2Fx8qvLy0zAJTNJFVgWfdIdoS4=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=2SFmbM5HxkVpGZlzG1ndI3TMSmGOegr72gop/M3d73kIg1CFZnKxGtGcKyezck+mZ O+kCIqz76ma3ykjBvmEgHBffAgre35RBYSQrrVwnng/jiCNp1fOpZJttEX9hcKFqey qBPAkG7FegbGwpqDFZNgK/OzszfouwU5/DBHuJXs= Message-ID: Subject: Re: [PATCH] locks: Fix UBSAN undefined behaviour in flock64_to_posix_lock From: Jeff Layton To: Eric Biggers , Luo Meng Cc: bfields@fieldses.org, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org Date: Thu, 22 Oct 2020 13:48:58 -0400 In-Reply-To: <20201022172500.GA3613750@gmail.com> References: <20201022020341.2434316-1-luomeng12@huawei.com> <20201022172500.GA3613750@gmail.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.5 (3.36.5-1.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Thu, 2020-10-22 at 10:25 -0700, Eric Biggers wrote: > On Thu, Oct 22, 2020 at 10:03:41AM +0800, Luo Meng wrote: > > When the sum of fl->fl_start and l->l_len overflows, > > UBSAN shows the following warning: > > > > UBSAN: Undefined behaviour in fs/locks.c:482:29 > > signed integer overflow: 2 + 9223372036854775806 > > cannot be represented in type 'long long int' > > Call Trace: > > __dump_stack lib/dump_stack.c:77 [inline] > > dump_stack+0xe4/0x14e lib/dump_stack.c:118 > > ubsan_epilogue+0xe/0x81 lib/ubsan.c:161 > > handle_overflow+0x193/0x1e2 lib/ubsan.c:192 > > flock64_to_posix_lock fs/locks.c:482 [inline] > > flock_to_posix_lock+0x595/0x690 fs/locks.c:515 > > fcntl_setlk+0xf3/0xa90 fs/locks.c:2262 > > do_fcntl+0x456/0xf60 fs/fcntl.c:387 > > __do_sys_fcntl fs/fcntl.c:483 [inline] > > __se_sys_fcntl fs/fcntl.c:468 [inline] > > __x64_sys_fcntl+0x12d/0x180 fs/fcntl.c:468 > > do_syscall_64+0xc8/0x5a0 arch/x86/entry/common.c:293 > > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > > > Fix it by moving -1 forward. > > > > Signed-off-by: Luo Meng > > --- > > fs/locks.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/locks.c b/fs/locks.c > > index 1f84a03601fe..8489787ca97e 100644 > > --- a/fs/locks.c > > +++ b/fs/locks.c > > @@ -542,7 +542,7 @@ static int flock64_to_posix_lock(struct file *filp, struct file_lock *fl, > > if (l->l_len > 0) { > > if (l->l_len - 1 > OFFSET_MAX - fl->fl_start) > > return -EOVERFLOW; > > - fl->fl_end = fl->fl_start + l->l_len - 1; > > + fl->fl_end = fl->fl_start - 1 + l->l_len; > > > > Given what the bounds check just above does, wouldn't it make more sense to > parenthesize 'l->l_len - 1' instead? So: > > fl->fl_end = fl->fl_start + (l->l_len - 1); > > Also FWIW, the Linux kernel uses the -fwrapv compiler flag, so signed integer > overflow is defined. IMO it's still best avoided though... > That does seem less ambiguous. Luo, if you're OK with that approach, I can just fix it up in-tree. -- Jeff Layton