Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Mon, 26 Jun 2017, Jiri Slaby wrote:
> On 06/23/2017, 09:51 AM, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, Jiri Slaby wrote:
> >> diff --git a/arch/arm64/include/asm/futex.h
> >> b/arch/arm64/include/asm/futex.h
> >> index f32b42e8725d..5bb2fd4674e7 100644
> >> --- a/arch/arm64/include/asm/futex.h
> >> +++ b/arch/arm64/include/asm/futex.h
> >> @@ -48,20 +48,10 @@ do {
> >> \
> >> } while (0)
> >>
> >> static inline int
> >> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> >
> > That unsigned int seems to be a change from the arm64 tree in next. It's
> > not upstream and it'll cause a (easy to resolve) conflict.
>
> Ugh, I thought the arm64 is in upstream already. Note that this patch
> just takes what is in this arm64 fix and makes it effective for all
> architectures. So I will wait with v2 until it merges upstream.
Ok.
> > Yes, we probably can't change that anymore, but at least we should make it
> > very explicit and add a comment to that effect.
>
> Something like this or do you want a comment yet?
> unsigned int op = (encoded_op & 0x7000) >> 28;
> unsigned int cmp =(encoded_op & 0x0f00) >> 24;
> int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
> int cmparg = sign_extend32(encoded_op & 0x0fff, 12);
Yes, that makes sense.
There is also the issue with the shift. See this thread for further
reference:
http://lkml.kernel.org/r/alpine.DEB.2.20.1706282353190.1890@nanos
The gist is:
"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"
So we should make that case explicit as well.
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
if (oparg < 0 || oparg > 31)
return -EINVAL;
oparg = 1 << oparg;
}
Thanks,
tglx
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Mon, Jun 26, 2017 at 02:02:31PM +0200, Jiri Slaby wrote:
> On 06/23/2017, 09:51 AM, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, Jiri Slaby wrote:
> >> diff --git a/arch/arm64/include/asm/futex.h
> >> b/arch/arm64/include/asm/futex.h
> >> index f32b42e8725d..5bb2fd4674e7 100644
> >> --- a/arch/arm64/include/asm/futex.h
> >> +++ b/arch/arm64/include/asm/futex.h
> >> @@ -48,20 +48,10 @@ do {
> >> \
> >> } while (0)
> >>
> >> static inline int
> >> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> >
> > That unsigned int seems to be a change from the arm64 tree in next. It's
> > not upstream and it'll cause a (easy to resolve) conflict.
>
> Ugh, I thought the arm64 is in upstream already. Note that this patch
> just takes what is in this arm64 fix and makes it effective for all
> architectures. So I will wait with v2 until it merges upstream.
>
> So, Will, will you incorporate Thomas' comments into your arm64 fix?
I wasn't planning to (it's already queued and I think they're just cosmetic
changes). The easiest thing is probably for you to make the changes in the
generic version when you post v2.
Will
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On 06/23/2017, 09:51 AM, Thomas Gleixner wrote:
> On Wed, 21 Jun 2017, Jiri Slaby wrote:
>> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
>> index f32b42e8725d..5bb2fd4674e7 100644
>> --- a/arch/arm64/include/asm/futex.h
>> +++ b/arch/arm64/include/asm/futex.h
>> @@ -48,20 +48,10 @@ do {
>> \
>> } while (0)
>>
>> static inline int
>> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
>
> That unsigned int seems to be a change from the arm64 tree in next. It's
> not upstream and it'll cause a (easy to resolve) conflict.
Ugh, I thought the arm64 is in upstream already. Note that this patch
just takes what is in this arm64 fix and makes it effective for all
architectures. So I will wait with v2 until it merges upstream.
So, Will, will you incorporate Thomas' comments into your arm64 fix?
...
> Yes, we probably can't change that anymore, but at least we should make it
> very explicit and add a comment to that effect.
Something like this or do you want a comment yet?
unsigned int op = (encoded_op & 0x7000) >> 28;
unsigned int cmp =(encoded_op & 0x0f00) >> 24;
int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
int cmparg = sign_extend32(encoded_op & 0x0fff, 12);
thanks,
--
js
suse labs
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Wed, 21 Jun 2017, Jiri Slaby wrote:
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index f32b42e8725d..5bb2fd4674e7 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -48,20 +48,10 @@ do {
> \
> } while (0)
>
> static inline int
> -futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
That unsigned int seems to be a change from the arm64 tree in next. It's
not upstream and it'll cause a (easy to resolve) conflict.
> +static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
> +{
> + int op = (encoded_op >> 28) & 7;
> + int cmp = (encoded_op >> 24) & 15;
> + int oparg = (int)(encoded_op << 8) >> 20;
> + int cmparg = (int)(encoded_op << 20) >> 20;
So this is really bad. We have implicit and explicit type casting to
int. And while we are at it can we please stop proliferating the existing
mess.
'op' and 'cmp' definitly can be unsigned int. There is no reason to cast
them to int.
oparg, cmparg and oldval are more interesting.
The logic here is "documented" in uapi/linux/futex.h
/* FUTEX_WAKE_OP will perform atomically
int oldval = *(int *)UADDR2;
*(int *)UADDR2 = oldval OP OPARG;
if (oldval CMP CMPARG)
wake UADDR2; */
Now the FUTEX_OP macro which is supposed to compose the encoded_up does:
#define FUTEX_OP(op, oparg, cmp, cmparg) \
(((op & 0xf) << 28) | ((cmp & 0xf) << 24) \
| ((oparg & 0xfff) << 12) | (cmparg & 0xfff))
Of course this all is not typed, undocumented and completely ill
defined.
> + int oparg = (int)(encoded_op << 8) >> 20;
> + int cmparg = (int)(encoded_op << 20) >> 20;
So in fact we sign expand the 12 bits of oparg and cmparg. Really
intuitive.
Yes, we probably can't change that anymore, but at least we should make it
very explicit and add a comment to that effect.
Thanks,
tglx
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 1/1] futex: remove duplicated code and fix UB
On Wed, Jun 21, 2017 at 01:53:18PM +0200, Jiri Slaby wrote:
> There is code duplicated over all architecture's headers for
> futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,
> and comparison of the result.
>
> Remove this duplication and leave up to the arches only the needed
> assembly which is now in arch_futex_atomic_op_inuser.
>
> This effectively distributes the Will Deacon's arm64 fix for undefined
> behaviour reported by UBSAN to all architectures. The fix was done in
> commit 5f16a046f8e1 (arm64: futex: Fix undefined behaviour with
> FUTEX_OP_OPARG_SHIFT usage). Look there for an example dump.
>
> Note that s390 removed access_ok check in d12a29703 ("s390/uaccess:
> remove pointless access_ok() checks") as access_ok there returns true.
> We introduce it back to the helper for the sake of simplicity (it gets
> optimized away anyway).
>
This required a minor manual merge for ARM on the tip of Linus' tree today. The
reduced duplication is a welcome improvement.
Reviewed-by: Darren Hart (VMware)
--
Darren Hart
VMware Open Source Technology Center
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
