linux-kernel - Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <568AC6DF-7E6D-4F10-BD41-D43195629C13@zytor.com>
Date:   Wed, 28 Jun 2017 14:43:46 -0700
From:   hpa@...or.com
To:     zhong jiang <zhongjiang@...wei.com>, Ingo Molnar <mingo@...nel.org>
CC:     akpm@...ux-foundation.org, tglx@...utronix.de, mingo@...hat.com,
        minchan@...nel.org, mhocko@...e.com, x86@...nel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        zhongjiang <zhongjiang@...wei.com>
Subject: Re: [PATCH] futex: avoid undefined behaviour when shift exponent is negative

On June 27, 2017 9:35:10 PM PDT, zhong jiang <zhongjiang@...wei.com> wrote:
>Hi,  Ingo
>
>Thank you for the comment.
>On 2017/6/22 0:40, Ingo Molnar wrote:
>> * zhong jiang <zhongjiang@...wei.com> wrote:
>>
>>> when shift expoment is negative, left shift alway zero. therefore,
>we
>>> modify the logic to avoid the warining.
>>>
>>> Signed-off-by: zhong jiang <zhongjiang@...wei.com>
>>> ---
>>>  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.
>  as the above case,  oparg is a negative is common. 
>
> I think it can be avoided by following change. 
>
>diff --git a/arch/x86/include/asm/futex.h
>b/arch/x86/include/asm/futex.h
>index b4c1f54..3205e86 100644
>--- a/arch/x86/include/asm/futex.h
>+++ b/arch/x86/include/asm/futex.h
>@@ -50,7 +50,7 @@ static inline int futex_atomic_op_inuser(int
>encoded_op, u32 __user *uaddr)
>        int oldval = 0, ret, tem;
>
>        if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
>-               oparg = 1 << oparg;
>+               oparg = safe_shift(1, oparg);
>
>        if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
>                return -EFAULT;
>diff --git a/drivers/video/fbdev/core/fbmem.c
>b/drivers/video/fbdev/core/fbmem.c
>index 069fe79..b4edda3 100644
>--- a/drivers/video/fbdev/core/fbmem.c
>+++ b/drivers/video/fbdev/core/fbmem.c
>@@ -190,11 +190,6 @@ char* fb_get_buffer_offset(struct fb_info *info,
>struct fb_pixmap *buf, u32 size
>
> #ifdef CONFIG_LOGO
>
>-static inline unsigned safe_shift(unsigned d, int n)
>-{
>-       return n < 0 ? d >> -n : d << n;
>-}
>-
> static void fb_set_logocmap(struct fb_info *info,
>                                   const struct linux_logo *logo)
> {
>diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>index d043ada..f3b8856 100644
>--- a/include/linux/kernel.h
>+++ b/include/linux/kernel.h
>@@ -841,6 +841,10 @@ static inline void ftrace_dump(enum
>ftrace_dump_mode oops_dump_mode) { }
>  */
> #define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
>
>+static inline unsigned safe_shift(unsigned d, int n)
>+{
>+       return n < 0 ? d >> -n : d << n;
>+}
>
>Thansk
>zhongjiang
>
>> Thanks,
>>
>> 	Ingo
>>
>> .
>>

What makes it reasonable?  It is totally ill-defined and doesn't do anything useful now?
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.