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

[hpa]

On June 28, 2017 7:12:04 PM PDT, zhong jiang <zhongjiang@xxxxxxxxxx> wrote:
>On 2017/6/29 5:43, hpa@xxxxxxxxx wrote:
>> On June 27, 2017 9:35:10 PM PDT, zhong jiang <zhongjiang@xxxxxxxxxx>
>wrote:
>>> Hi,  Ingo
>>>
>>> Thank you for the comment.
>>> On 2017/6/22 0:40, Ingo Molnar wrote:
>>>> * zhong jiang <zhongjiang@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>>  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?
> Thanks you for comments.
> 
>Maybe I mismake the meaning. I test the negative cases in x86 , all
>case is zero. so I come to a conclusion.
> 
>zj.c:15:8: warning: left shift count is negative
>[-Wshift-count-negative]
>  j = 1 << -2048;
>        ^
>[root@localhost zhongjiang]# ./zj
>j = 0
>j.c:15:8: warning: left shift count is negative
>[-Wshift-count-negative]
>  j = 1 << -2047;
>        ^
>[root@localhost zhongjiang]# ./zj
>j = 0
>
>I insmod a module into kernel to test the testcasts, all of the result
>is zero.
>
>I wonder whether I miss some point or not. Do you point out to me?
>please
>
>Thanks
>zhongjiang
> 
> 

When you use compile-time constants, the compiler generates the value at compile time, which can be totally different.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.