Re: [RFC]: Possible race condition in kernel futex code

Re: [RFC]: Possible race condition in kernel futex code

[Thomas Gleixner]

On Mon, 5 Oct 2015, Jaccon Bastiaansen wrote:
> We did some tests with different compilers, kernel versions and kernel
> configs, with the following results:
> 
> Linux 3.12.48, x86_64_defconfig, GCC 4.6.1 :
> copy_user_generic_unrolled being used, so race condition possible
> Linux 3.12.48, x86_64_defconfig, GCC 4.9.1 :
> copy_user_generic_unrolled being used, so race condition possible
> Linux 4.2.3, x86_64_defconfig, GCC 4.6.1 : 32 bit read being used, no
> race condition
> Linux 4.2.3, x86_64_defconfig, GCC 4.9.1 : 32 bit read being used, no
> race condition
> 
> 
> Our idea to fix this problem is use an explicit 32 bit read in
> get_futex_value_locked() instead of using the generic function
> copy_from_user_inatomic() and hoping the compiler uses an atomic
> access and the right access size.

You cannot use an explicit 32bit read. We need an access which handles
the fault gracefully.

In current mainline this is done proper:

ret = __copy_from_user_inatomic(dst, src, size = sizeof(u32))

        __copy_from_user_nocheck(dst, src, size)

    	       if (!__builtin_constant_p(size))
                     return copy_user_generic(dst, (__force void *)src, size);
	
	       size is constant so we end up in the switch case

	       switch(size) {
	       
	       case 4:
	       	    __get_user_asm(*(u32 *)dst, (u32 __user *)src,
		     		   ret, "l", "k", "=r", 4);
		    return ret;
....

In 3.12 this is different:

   __copy_from_user_inatomic()
	copy_user_generic()
	    copy_user_generic_unrolled()

So this is only an issue for kernel versions < 3.13. It was fixed with

ff47ab4ff3cd: Add 1/2/4/8 byte optimization to 64bit __copy_{from,to}_user_inatomic

but nobody noticed that the race you described can happen, so it was
never backported to the stable kernels.

@stable: Can you please pick up ff47ab4ff3cd plus 

df90ca969035d x86, sparse: Do not force removal of __user when calling copy_to/from_user_nocheck()

for stable kernels <= 3.12?

If that's too much of churn, then I can come up with an explicit fix
for this. Let me know.

Thanks,

	tglx

Re: [RFC]: Possible race condition in kernel futex code

[Hans Zuidam]

Hi Thomas,

On 9 okt. 2015, at 11:06, Thomas Gleixner <tglx@linutronix.de> wrote:
On Mon, 5 Oct 2015, Jaccon Bastiaansen wrote:
>> We did some tests with different compilers, kernel versions and kernel
>> configs, with the following results:

> You cannot use an explicit 32bit read. We need an access which handles the fault gracefully.

The reason for the explicit read suggestion is to avoid the _builtin_constant_p() in __copy_from_user_nocheck().  The GCC manual says that there may be situations where it returns 0 even though the argument is a constant.  Although none of the compiler/kernel combinations we have tried showed this happening, we think it is probably better to be safe than sorry.

With kind regards,
Hans Zuidam

Re: [RFC]: Possible race condition in kernel futex code

[Thomas Gleixner]

Hans,

On Fri, 9 Oct 2015, Hans Zuidam wrote:
> On 9 okt. 2015, at 11:06, Thomas Gleixner <tglx@linutronix.de> wrote:
> > You cannot use an explicit 32bit read. We need an access which
> > handles the fault gracefully.
>
> The reason for the explicit read suggestion is to avoid the
> _builtin_constant_p() in __copy_from_user_nocheck().  The GCC manual
> says that there may be situations where it returns 0 even though the
> argument is a constant.

That's insane at best.

> Although none of the compiler/kernel combinations we have tried
> showed this happening, we think it is probably better to be safe
> than sorry.

So we would need something like:

   futex_copy_from_user()

which can be mapped to __copy_from_user_inatomic() first. Then go
through all architectures and the asm-generic stuff and provide the
specific variants which are guaranteed to use a 32bit access.

I really prefer that we don't have to do that and the compiler people
get their act together and fix that _builtin_constant_p() thingy.

Thanks,

	tglx

Re: [RFC]: Possible race condition in kernel futex code

[Peter Zijlstra]

On Fri, Oct 09, 2015 at 11:25:09AM +0100, Thomas Gleixner wrote:
> Hans,
> 
> On Fri, 9 Oct 2015, Hans Zuidam wrote:
> > On 9 okt. 2015, at 11:06, Thomas Gleixner <tglx@linutronix.de> wrote:
> > > You cannot use an explicit 32bit read. We need an access which
> > > handles the fault gracefully.
> >
> > The reason for the explicit read suggestion is to avoid the
> > _builtin_constant_p() in __copy_from_user_nocheck().  The GCC manual
> > says that there may be situations where it returns 0 even though the
> > argument is a constant.
> 
> That's insane at best.

Right, but I bet that is for cases where constant propagation completely
fails, and this is a trivial case, I have no problem relying on it.

Re: [RFC]: Possible race condition in kernel futex code

[Greg KH]

On Fri, Oct 09, 2015 at 10:06:41AM +0100, Thomas Gleixner wrote:
> On Mon, 5 Oct 2015, Jaccon Bastiaansen wrote:
> > We did some tests with different compilers, kernel versions and kernel
> > configs, with the following results:
> > 
> > Linux 3.12.48, x86_64_defconfig, GCC 4.6.1 :
> > copy_user_generic_unrolled being used, so race condition possible
> > Linux 3.12.48, x86_64_defconfig, GCC 4.9.1 :
> > copy_user_generic_unrolled being used, so race condition possible
> > Linux 4.2.3, x86_64_defconfig, GCC 4.6.1 : 32 bit read being used, no
> > race condition
> > Linux 4.2.3, x86_64_defconfig, GCC 4.9.1 : 32 bit read being used, no
> > race condition
> > 
> > 
> > Our idea to fix this problem is use an explicit 32 bit read in
> > get_futex_value_locked() instead of using the generic function
> > copy_from_user_inatomic() and hoping the compiler uses an atomic
> > access and the right access size.
> 
> You cannot use an explicit 32bit read. We need an access which handles
> the fault gracefully.
> 
> In current mainline this is done proper:
> 
> ret = __copy_from_user_inatomic(dst, src, size = sizeof(u32))
> 
>         __copy_from_user_nocheck(dst, src, size)
> 
>     	       if (!__builtin_constant_p(size))
>                      return copy_user_generic(dst, (__force void *)src, size);
> 	
> 	       size is constant so we end up in the switch case
> 
> 	       switch(size) {
> 	       
> 	       case 4:
> 	       	    __get_user_asm(*(u32 *)dst, (u32 __user *)src,
> 		     		   ret, "l", "k", "=r", 4);
> 		    return ret;
> ....
> 
> In 3.12 this is different:
> 
>    __copy_from_user_inatomic()
> 	copy_user_generic()
> 	    copy_user_generic_unrolled()
> 
> So this is only an issue for kernel versions < 3.13. It was fixed with
> 
> ff47ab4ff3cd: Add 1/2/4/8 byte optimization to 64bit __copy_{from,to}_user_inatomic
> 
> but nobody noticed that the race you described can happen, so it was
> never backported to the stable kernels.
> 
> @stable: Can you please pick up ff47ab4ff3cd plus 
> 
> df90ca969035d x86, sparse: Do not force removal of __user when calling copy_to/from_user_nocheck()
> 
> for stable kernels <= 3.12?
> 
> If that's too much of churn, then I can come up with an explicit fix
> for this. Let me know.

Now applied to 3.10-stable, thanks.

greg k-h

Re: [RFC]: Possible race condition in kernel futex code

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Fri, 2015-10-09 at 10:06 +0100, Thomas Gleixner wrote:
[...]
> @stable: Can you please pick up ff47ab4ff3cd plus 
> 
> df90ca969035d x86, sparse: Do not force removal of __user when calling copy_to/from_user_nocheck()
> 
> for stable kernels <= 3.12?
[...]

I've finally queued these up for 3.2, thanks.

Ben.

-- 
Ben Hutchings
For every action, there is an equal and opposite criticism. - Harrison

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]