Re: [PATCH 4.4 099/190] arm64: futex: Fix undefined behaviour with FUTEX_OP_OPARG_SHIFT usage

2018-05-17 Thread Greg Kroah-Hartman
On Wed, May 16, 2018 at 12:15:36AM +0100, Ben Hutchings wrote:
> On Wed, 2018-04-11 at 20:35 +0200, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > > From: Will Deacon 
> > 
> > 
> > [ Upstream commit 5f16a046f8e144c294ef98cd29d9458b5f8273e5 ]
> > 
> > FUTEX_OP_OPARG_SHIFT instructs the futex code to treat the 12-bit oparg
> > field as a shift value, potentially leading to a left shift value that
> > is negative or with an absolute value that is significantly larger then
> > the size of the type. UBSAN chokes with:
> [...]
> > Whilst I think this catches all of the issues, I'd much prefer to remove
> > this stuff, as I think it's unused and the bugs are copy-pasted between
> > a bunch of architectures.
> [...]
> 
> Indeed.  That more complete fix was done upstream by:
> 
> commit 30d6e0a4190d37740e9447e4e4815f06992dd8c3
> Author: Jiri Slaby 
> Date:   Thu Aug 24 09:31:05 2017 +0200
> 
> futex: Remove duplicated code and fix undefined behaviour
> 
> It's a bit big for stable - though most of the changes are deletions. 
> What do you think?

I think it makes a lot of sense, I've done the backport now, thanks.

greg k-h


Re: [PATCH 4.4 099/190] arm64: futex: Fix undefined behaviour with FUTEX_OP_OPARG_SHIFT usage

2018-05-15 Thread Ben Hutchings
On Wed, 2018-04-11 at 20:35 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> > From: Will Deacon 
> 
> 
> [ Upstream commit 5f16a046f8e144c294ef98cd29d9458b5f8273e5 ]
> 
> FUTEX_OP_OPARG_SHIFT instructs the futex code to treat the 12-bit oparg
> field as a shift value, potentially leading to a left shift value that
> is negative or with an absolute value that is significantly larger then
> the size of the type. UBSAN chokes with:
[...]
> Whilst I think this catches all of the issues, I'd much prefer to remove
> this stuff, as I think it's unused and the bugs are copy-pasted between
> a bunch of architectures.
[...]

Indeed.  That more complete fix was done upstream by:

commit 30d6e0a4190d37740e9447e4e4815f06992dd8c3
Author: Jiri Slaby 
Date:   Thu Aug 24 09:31:05 2017 +0200

futex: Remove duplicated code and fix undefined behaviour

It's a bit big for stable - though most of the changes are deletions. 
What do you think?

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.



[PATCH 4.4 099/190] arm64: futex: Fix undefined behaviour with FUTEX_OP_OPARG_SHIFT usage

2018-04-11 Thread Greg Kroah-Hartman
4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Will Deacon 


[ Upstream commit 5f16a046f8e144c294ef98cd29d9458b5f8273e5 ]

FUTEX_OP_OPARG_SHIFT instructs the futex code to treat the 12-bit oparg
field as a shift value, potentially leading to a left shift value that
is negative or with an absolute value that is significantly larger then
the size of the type. UBSAN chokes with:


UBSAN: Undefined behaviour in ./arch/arm64/include/asm/futex.h:60:13
shift exponent -1 is negative
CPU: 1 PID: 1449 Comm: syz-executor0 Not tainted 
4.11.0-rc4-5-g977eb52-dirty #11
Hardware name: linux,dummy-virt (DT)
Call trace:
[] dump_backtrace+0x0/0x538 arch/arm64/kernel/traps.c:73
[] show_stack+0x20/0x30 arch/arm64/kernel/traps.c:228
[] __dump_stack lib/dump_stack.c:16 [inline]
[] dump_stack+0x120/0x188 lib/dump_stack.c:52
[] ubsan_epilogue+0x18/0x98 lib/ubsan.c:164
[] __ubsan_handle_shift_out_of_bounds+0x250/0x294 
lib/ubsan.c:421
[] futex_atomic_op_inuser arch/arm64/include/asm/futex.h:60 
[inline]
[] futex_wake_op kernel/futex.c:1489 [inline]
[] do_futex+0x137c/0x1740 kernel/futex.c:3231
[] SYSC_futex kernel/futex.c:3281 [inline]
[] SyS_futex+0x114/0x268 kernel/futex.c:3249
[] el0_svc_naked+0x24/0x28

syz-executor1 uses obsolete (PF_INET,SOCK_PACKET)
sock: process `syz-executor0' is using obsolete setsockopt SO_BSDCOMPAT

This patch attempts to fix some of this by:

  * Making encoded_op an unsigned type, so we can shift it left even if
the top bit is set.

  * Casting to signed prior to shifting right when extracting oparg
and cmparg

  * Consider only the bottom 5 bits of oparg when using it as a left-shift
value.

Whilst I think this catches all of the issues, I'd much prefer to remove
this stuff, as I think it's unused and the bugs are copy-pasted between
a bunch of architectures.

Reviewed-by: Robin Murphy 
Signed-off-by: Will Deacon 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 arch/arm64/include/asm/futex.h |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

--- a/arch/arm64/include/asm/futex.h
+++ b/arch/arm64/include/asm/futex.h
@@ -53,16 +53,16 @@
: "memory")
 
 static inline int
-futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
+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 = (encoded_op << 8) >> 20;
-   int cmparg = (encoded_op << 20) >> 20;
+   int oparg = (int)(encoded_op << 8) >> 20;
+   int cmparg = (int)(encoded_op << 20) >> 20;
int oldval = 0, ret, tmp;
 
if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28))
-   oparg = 1 << oparg;
+   oparg = 1U << (oparg & 0x1f);
 
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
return -EFAULT;