| 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
| ||
|
Date: Fri, 08 Jul 2011 10:07:57 -0700
From: Darren Hart <dvhart@...ux.intel.com>
To: Vitaliy Ivanov <vitalivanov@...il.com>
CC: Jiri Kosina <jkosina@...e.cz>,
Thomas Gleixner <tglx@...utronix.de>,
Linus Torvalds <torvalds@...ux-foundation.org>,
lkml <linux-kernel@...r.kernel.org>,
"trivial@...nel.org" <trivial@...nel.org>
Subject: Re: [PATCH 4/4] futex: warning corrections
On 07/08/2011 08:00 AM, Vitaliy Ivanov wrote:
> On Thu, 2011-07-07 at 11:06 -0700, Darren Hart wrote:
>>
>> On 07/07/2011 05:39 AM, Vitaliy Ivanov wrote:
>>>>> From 8eeaa5a97697bcc606aea23d32028aea7b271a96 Mon Sep 17 00:00:00 2001
>>>>> From: Vitaliy Ivanov <vitalivanov@...il.com>
>>>>> Date: Thu, 7 Jul 2011 00:05:05 +0300
>>>>> Subject: [PATCH] futex: uninitialized warning corrections
>>>>> MIME-Version: 1.0
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>> Content-Transfer-Encoding: 8bit
>>>>>
>>>>> kernel/futex.c: In function ‘fixup_pi_state_owner.clone.17’:
>>>>> kernel/futex.c:1582:6: warning: ‘curval’ may be used uninitialized in this function
>>>>> kernel/futex.c: In function ‘handle_futex_death’:
>>>>> kernel/futex.c:2486:6: warning: ‘nval’ may be used uninitialized in this function
>>>>> kernel/futex.c: In function ‘do_futex’:
>>>>> kernel/futex.c:863:11: warning: ‘curval’ may be used uninitialized in this function
>>>>> kernel/futex.c:828:6: note: ‘curval’ was declared here
>>>>> kernel/futex.c:898:5: warning: ‘oldval’ may be used uninitialized in this function
>>>>> kernel/futex.c:890:6: note: ‘oldval’ was declared here
>>>>>
>>>>> Signed-off-by: Vitaliy Ivanov <vitalivanov@...il.com>
>>>>
>>>> Please include a blurb in the commit message as to why you used
>>>> uninitialized_var() rather than just assigning it. This will save people
>>>> the time of wondering why, and me the time of nacking "it's simpler to
>>>> just initialize to zero" patches :-)
>>>>
>>>> Acked-by: Darren Hart <dvhart@...ux.intel.com>
>>>
>>> Darren,
>>>
>>> Thanks for your comments. I think the description is pretty obvious
>>> here as I don't think any of these variables are affected by cmpxchg.
>>
>> Not so. Consider the following:
>>
>> u32 curval;
>> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
>> ret = -EFAULT;
>> else if (curval != uval)
>> ret = -EINVAL;
>>
>> the cmpxchg here assigns curval to newval if *uaddr==uval or to *uaddr
>> otherwise. This is where curval gets assigned so that it can then be
>> read in the following if block. gcc didn't recognize this as an
>> assignment and is why it complained about it being used uninitialized.
>>
>>
>>> There is simple assignment at the end. Seems like compiler simply
>>> doesn't follow all the return cases.
>>
>> No, the compiler complained about the test of the value, this doesn't
>> have anything to do with the return cases.
>
> Here is what we have:
>
> ------------
> static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
> struct task_struct *newowner)
> {
> u32 curval;
> ...
> retry:
> if (get_futex_value_locked(&uval, uaddr))
> goto handle_fault;
>
> while (1) {
> newval = (uval & FUTEX_OWNER_DIED) | newtid;
>
> if (cmpxchg_futex_value_locked(&curval, uaddr, uval, newval))
> goto handle_fault;
> if (curval == uval)
> break;
> uval = curval;
> }
> ...
> }
>
> ------------
> static int cmpxchg_futex_value_locked(u32 *curval, u32 __user *uaddr,
> u32 uval, u32 newval)
> {
> int ret;
>
> pagefault_disable();
> ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval);
> pagefault_enable();
>
> return ret;
> }
> ------------
>
> And for x86:
>
> static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> u32 oldval, u32 newval)
> {
> int ret = 0;
>
> #if defined(CONFIG_X86_32) && !defined(CONFIG_X86_BSWAP)
> /* Real i386 machines have no cmpxchg instruction */
> if (boot_cpu_data.x86 == 3)
> return -ENOSYS;
> #endif
>
> if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
> return -EFAULT;
>
> asm volatile("1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n"
> "2:\t.section .fixup, \"ax\"\n"
> "3:\tmov %3, %0\n"
> "\tjmp 2b\n"
> "\t.previous\n"
> _ASM_EXTABLE(1b, 3b)
> : "+r" (ret), "=a" (oldval), "+m" (*uaddr)
> : "i" (-EFAULT), "r" (newval), "1" (oldval)
> : "memory"
> );
>
> *uval = oldval; <------------- uval is being changed here only. it's not modified by asm cmpxchgl.
> return ret;
> }
>
>
> Am I missing something?
Nope, I was incorrect - I didn't realize that
futex_atomic_cmpxchg_inatomic didn't set uval on EFAULT. gcc is not
detecting that curval is only read if futex_atomic_cmpxchg_inatomic
succeeds, so I believe the uninitialized_var() fix is correct. This is
one of the reasons why we need to include the reasoning in the commit
log - specifically that:
"futex_atomic_cmpxchg_inatomic only assigns curval on success, but on
failure curval is not read, so instruct gcc to ignore the uninitialized
warning."
--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists