Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751879AbdHYFWd (ORCPT ); Fri, 25 Aug 2017 01:22:33 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:4571 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751453AbdHYFWb (ORCPT ); Fri, 25 Aug 2017 01:22:31 -0400 Message-ID: <599FB3C4.6000009@huawei.com> Date: Fri, 25 Aug 2017 13:21:08 +0800 From: zhong jiang User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1 MIME-Version: 1.0 To: Thomas Gleixner , Ingo Molnar CC: , , , , , , , , Zhen Lei Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative References: <1498045437-7675-1-git-send-email-zhongjiang@huawei.com> <20170621164036.4findvvz7jj4cvqo@gmail.com> <595331FE.3090700@huawei.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.29.68] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A0B0205.599FB3D2.00A6,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: ad199aa14f52ac82acf718f8ca7d2531 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5305 Lines: 124 On 2017/6/29 6:13, Thomas Gleixner wrote: > On Wed, 28 Jun 2017, zhong jiang wrote: >> On 2017/6/22 0:40, Ingo Molnar wrote: >>> * zhong jiang wrote: >>> >>>> when shift expoment is negative, left shift alway zero. therefore, we >>>> modify the logic to avoid the warining. >>>> >>>> Signed-off-by: zhong jiang >>>> --- >>>> arch/x86/include/asm/futex.h | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/x86/include/asm/futex.h b/arch/x86/include/asm/futex.h >>>> index b4c1f54..2425fca 100644 >>>> --- a/arch/x86/include/asm/futex.h >>>> +++ b/arch/x86/include/asm/futex.h >>>> @@ -49,8 +49,12 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr) >>>> int cmparg = (encoded_op << 20) >> 20; >>>> int oldval = 0, ret, tem; >>>> >>>> - if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) >>>> - oparg = 1 << oparg; >>>> + if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) { >>>> + if (oparg >= 0) >>>> + oparg = 1 << oparg; >>>> + else >>>> + oparg = 0; >>>> + } >>> Could we avoid all these complications by using an unsigned type? >> I think it is not feasible. a negative shift exponent is likely >> existence and reasonable. > What is reasonable about a negative shift value? > >> as the above case, oparg is a negative is common. > That's simply wrong. If oparg is negative and the SHIFT bit is set then the > result is undefined today and there is no way that this can be used at > all. > > On x86: > > 1 << -1 = 0x80000000 > 1 << -2048 = 0x00000001 > 1 << -2047 = 0x00000002 > > Anything using a shift value < 0 or > 31 will get crap as a > result. Rightfully so because it's just undefined. > > Yes I know that the insanity of user space is unlimited, but anything > attempting this is so broken that we cannot break it further by making that > shift arg unsigned and actually limit it to 0-31 > > Thanks, > > tglx > > > > . > >From df9e2a5a3f1f401943aeb2718d5876b854dea3a3 Mon Sep 17 00:00:00 2001 From: zhong jiang Date: Fri, 25 Aug 2017 12:05:56 +0800 Subject: [PATCH v2] futex: avoid undefined behaviour when shift exponent is negative when futex syscall is called from userspace, we find the following warning by ubsan detection. [ 63.237803] UBSAN: Undefined behaviour in /root/rpmbuild/BUILDROOT/kernel-3.10.0-327.49.58.52.x86_64/usr/src/linux-3.10.0-327.49.58.52.x86_64/arch/x86/include/asm/futex.h:53:13 [ 63.237803] shift exponent -16 is negative [ 63.237803] CPU: 0 PID: 67 Comm: driver Not tainted 3.10.0 #1 [ 63.237803] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 04/01/2014 [ 63.237803] fffffffffffffff0 000000009ad70fde ffff88000002fa08 ffffffff81ef0d6f [ 63.237803] ffff88000002fa20 ffffffff81ef0e2c ffffffff828f2540 ffff88000002fb90 [ 63.237803] ffffffff81ef1ad0 ffffffff8141cc88 1ffff10000005f48 0000000041b58ab3 [ 63.237803] Call Trace: [ 63.237803] [] dump_stack+0x1e/0x20 [ 63.237803] [] ubsan_epilogue+0x12/0x55 [ 63.237803] [] __ubsan_handle_shift_out_of_bounds+0x237/0x29c [ 63.237803] [] ? kasan_alloc_pages+0x38/0x40 [ 63.237803] [] ? __ubsan_handle_load_invalid_value+0x162/0x162 [ 63.237803] [] ? get_futex_key+0x361/0x6c0 [ 63.237803] [] ? get_futex_key_refs+0xb0/0xb0 [ 63.237803] [] futex_wake_op+0xb48/0xc70 [ 63.237803] [] ? futex_wake_op+0xb48/0xc70 [ 63.237803] [] ? futex_wake+0x380/0x380 [ 63.237803] [] do_futex+0x2cc/0xb60 [ 63.237803] [] ? exit_robust_list+0x350/0x350 [ 63.237803] [] ? __fsnotify_inode_delete+0x20/0x20 [ 63.237803] [] ? n_tty_flush_buffer+0x80/0x80 [ 63.237803] [] ? __fsnotify_parent+0x53/0x210 [ 63.237803] [] SyS_futex+0x147/0x300 [ 63.237803] [] ? do_futex+0xb60/0xb60 [ 63.237803] [] ? do_page_fault+0x44/0xa0 [ 63.237803] [] system_call_fastpath+0x16/0x1b using a shift value < 0 or > 31 will get crap as a result. because it's just undefined. The issue still disturb me, so I try to fix it again by excluding the especially condition. Signed-off-by: zhong jiang --- arch/x86/include/asm/futex.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/futex.h b/arch/x86/include/asm/futex.h index b4c1f54..c414d76 100644 --- a/arch/x86/include/asm/futex.h +++ b/arch/x86/include/asm/futex.h @@ -49,9 +49,11 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr) int cmparg = (encoded_op << 20) >> 20; int oldval = 0, ret, tem; - if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) + if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) { + if (oparg < 0 || oparg > 31) + return -EINVAL; oparg = 1 << oparg; - + } if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) return -EFAULT;