linux-kernel - Re: [PATCH 1/1] futex: remove duplicated code and fix UB

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 3 Jul 2017 12:18:28 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Jiri Slaby <jslaby@...e.cz>
cc:     Will Deacon <will.deacon@....com>, mingo@...hat.com,
        peterz@...radead.org, dvhart@...radead.org,
        linux-kernel@...r.kernel.org, Richard Henderson <rth@...ddle.net>,
        Ivan Kokshaysky <ink@...assic.park.msu.ru>,
        Matt Turner <mattst88@...il.com>,
        Vineet Gupta <vgupta@...opsys.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Richard Kuo <rkuo@...eaurora.org>,
        Tony Luck <tony.luck@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Michal Simek <monstr@...str.eu>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Jonas Bonn <jonas@...thpole.se>,
        Stefan Kristiansson <stefan.kristiansson@...nalahti.fi>,
        Stafford Horne <shorne@...il.com>,
        "James E.J. Bottomley" <jejb@...isc-linux.org>,
        Helge Deller <deller@....de>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Rich Felker <dalias@...c.org>,
        "David S. Miller" <davem@...emloft.net>,
        "H. Peter Anvin" <hpa@...or.com>, Chris Zankel <chris@...kel.net>,
        Max Filippov <jcmvbkbc@...il.com>,
        Arnd Bergmann <arnd@...db.de>, x86@...nel.org,
        linux-alpha@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-hexagon@...r.kernel.org, linux-ia64@...r.kernel.org,
        linux-mips@...ux-mips.org, openrisc@...ts.librecores.org,
        linux-parisc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-s390@...r.kernel.org, linux-sh@...r.kernel.org,
        sparclinux@...r.kernel.org, linux-xtensa@...ux-xtensa.org,
        linux-arch@...r.kernel.org
Subject: 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 & 0x70000000) >> 28;
>         unsigned int cmp =        (encoded_op & 0x0f000000) >> 24;
>         int oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);
>         int cmparg = sign_extend32(encoded_op & 0x00000fff, 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