[{"id":1756142,"web_url":"http://patchwork.ozlabs.org/comment/1756142/","msgid":"<20170824094756.GA6346@arm.com>","list_archive_url":null,"date":"2017-08-24T09:47:56","subject":"Re: [PATCH v2 1/1] futex: remove duplicated code and fix UB","submitter":{"id":7916,"url":"http://patchwork.ozlabs.org/api/people/7916/","name":"Will Deacon","email":"will.deacon@arm.com"},"content":"Hi Jiri,\n\nOn Thu, Aug 24, 2017 at 09:31:05AM +0200, Jiri Slaby wrote:\n> There is code duplicated over all architecture's headers for\n> futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr,\n> and comparison of the result.\n> \n> Remove this duplication and leave up to the arches only the needed\n> assembly which is now in arch_futex_atomic_op_inuser.\n> \n> This effectively distributes the Will Deacon's arm64 fix for undefined\n> behaviour reported by UBSAN to all architectures. The fix was done in\n> commit 5f16a046f8e1 (arm64: futex: Fix undefined behaviour with\n> FUTEX_OP_OPARG_SHIFT usage). Look there for an example dump.\n> \n> And as suggested by Thomas, check for negative oparg too, because it was\n> also reported to cause undefined behaviour report.\n> \n> Note that s390 removed access_ok check in d12a29703 (\"s390/uaccess:\n> remove pointless access_ok() checks\") as access_ok there returns true.\n> We introduce it back to the helper for the sake of simplicity (it gets\n> optimized away anyway).\n\nFor arm64 and the core code:\n\nReviewed-by: Will Deacon \n\nAlthough one minor thing on the core part:\n\n> diff --git a/kernel/futex.c b/kernel/futex.c\n> index 0939255fc750..3d38eaf05492 100644\n> --- a/kernel/futex.c\n> +++ b/kernel/futex.c\n> @@ -1551,6 +1551,45 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset)\n> \treturn ret;\n> }\n> \n> +static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)\n> +{\n> +\tunsigned int op =\t (encoded_op & 0x70000000) >> 28;\n> +\tunsigned int cmp =\t (encoded_op & 0x0f000000) >> 24;\n> +\tint oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);\n> +\tint cmparg = sign_extend32(encoded_op & 0x00000fff, 12);\n> +\tint oldval, ret;\n> +\n> +\tif (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {\n> +\t\tif (oparg < 0 || oparg > 31)\n> +\t\t\treturn -EINVAL;\n> +\t\toparg = 1 << oparg;\n> +\t}\n> +\n> +\tif (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))\n> +\t\treturn -EFAULT;\n> +\n> +\tret = arch_futex_atomic_op_inuser(op, oparg, &oldval, uaddr);\n> +\tif (ret)\n> +\t\treturn ret;\n\nWe could move the pagefault_{disable,enable} calls here, and then remove\nthem from the futex_atomic_op_inuser callsites elsewhere in futex.c\n\nWill\n--\nTo unsubscribe from this list: send the line \"unsubscribe sparclinux\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=sparclinux-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xdKGm1vdGz9sR9\n\tfor ;\n\tThu, 24 Aug 2017 19:48:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752830AbdHXJr7 (ORCPT );\n\tThu, 24 Aug 2017 05:47:59 -0400","from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:38240 \"EHLO\n\tfoss.arm.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751264AbdHXJrz (ORCPT );\n\tThu, 24 Aug 2017 05:47:55 -0400","from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249])\n\tby usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5098815AD;\n\tThu, 24 Aug 2017 02:47:54 -0700 (PDT)","from edgewater-inn.cambridge.arm.com\n\t(usa-sjc-imap-foss1.foss.arm.com [10.72.51.249])\n\tby usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id\n\t1BC423F540; Thu, 24 Aug 2017 02:47:54 -0700 (PDT)","by edgewater-inn.cambridge.arm.com (Postfix, from userid 1000)\n\tid D5AEF1AE356E; Thu, 24 Aug 2017 10:47:56 +0100 (BST)"],"Date":"Thu, 24 Aug 2017 10:47:56 +0100","From":"Will Deacon ","To":"Jiri Slaby ","Cc":"tglx@linutronix.de, mingo@redhat.com, dvhart@infradead.org,\n\tpeterz@infradead.org, linux-kernel@vger.kernel.org,\n\tRichard Henderson ,\n\tIvan Kokshaysky ,\n\tMatt Turner , Vineet Gupta ,\n\tCatalin Marinas ,\n\tRichard Kuo , Tony Luck ,\n\tFenghua Yu , Michal Simek ,\n\tRalf Baechle , Jonas Bonn ,\n\tStefan Kristiansson ,\n\tStafford Horne ,\n\t\"James E.J. Bottomley\" ,\n\tHelge Deller ,\n\tBenjamin Herrenschmidt ,\n\tPaul Mackerras ,\n\tMartin Schwidefsky ,\n\tYoshinori Sato ,\n\tRich Felker , \"David S. Miller\" ,\n\t\"H. Peter Anvin\" , Chris Zankel ,\n\tMax Filippov ,\n\tArnd Bergmann , x86@kernel.org,\n\tlinux-alpha@vger.kernel.org, linux-snps-arc@lists.infradead.org,\n\tlinux-arm-kernel@lists.infradead.org,\n\tlinux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org,\n\tlinux-mips@linux-mips.org, openrisc@lists.librecores.org,\n\tlinux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,\n\tlinux-s390@vger.kernel.org, linux-sh@vger.kernel.org,\n\tsparclinux@vger.kernel.org, linux-xtensa@linux-xtensa.org,\n\tlinux-arch@vger.kernel.org","Subject":"Re: [PATCH v2 1/1] futex: remove duplicated code and fix UB","Message-ID":"<20170824094756.GA6346@arm.com>","References":"<20170824073105.3901-1-jslaby@suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170824073105.3901-1-jslaby@suse.cz>","User-Agent":"Mutt/1.5.23 (2014-03-12)","Sender":"sparclinux-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"sparclinux@vger.kernel.org"}},{"id":1757742,"web_url":"http://patchwork.ozlabs.org/comment/1757742/","msgid":"","list_archive_url":null,"date":"2017-08-25T20:43:41","subject":"Re: [PATCH v2 1/1] futex: remove duplicated code and fix UB","submitter":{"id":180,"url":"http://patchwork.ozlabs.org/api/people/180/","name":"Thomas Gleixner","email":"tglx@linutronix.de"},"content":"On Thu, 24 Aug 2017, Will Deacon wrote:\n> On Thu, Aug 24, 2017 at 09:31:05AM +0200, Jiri Slaby wrote:\n> > +static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)\n> > +{\n> > +\tunsigned int op =\t (encoded_op & 0x70000000) >> 28;\n> > +\tunsigned int cmp =\t (encoded_op & 0x0f000000) >> 24;\n> > +\tint oparg = sign_extend32((encoded_op & 0x00fff000) >> 12, 12);\n> > +\tint cmparg = sign_extend32(encoded_op & 0x00000fff, 12);\n> > +\tint oldval, ret;\n> > +\n> > +\tif (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {\n> > +\t\tif (oparg < 0 || oparg > 31)\n> > +\t\t\treturn -EINVAL;\n> > +\t\toparg = 1 << oparg;\n> > +\t}\n> > +\n> > +\tif (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))\n> > +\t\treturn -EFAULT;\n> > +\n> > +\tret = arch_futex_atomic_op_inuser(op, oparg, &oldval, uaddr);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> \n> We could move the pagefault_{disable,enable} calls here, and then remove\n> them from the futex_atomic_op_inuser callsites elsewhere in futex.c\n\nCorrect, but we can do that after getting this in.\n\nThanks,\n\n\ttglx\n--\nTo unsubscribe from this list: send the line \"unsubscribe sparclinux\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=sparclinux-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xfCqc4Zc6z9sNv\n\tfor ;\n\tSat, 26 Aug 2017 06:46:04 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932880AbdHYUpx (ORCPT );\n\tFri, 25 Aug 2017 16:45:53 -0400","from Galois.linutronix.de ([146.0.238.70]:42702 \"EHLO\n\tGalois.linutronix.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932836AbdHYUpt (ORCPT\n\t); Fri, 25 Aug 2017 16:45:49 -0400","from p5492fdbb.dip0.t-ipconnect.de ([84.146.253.187] helo=nanos)\n\tby Galois.linutronix.de with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1dlLR2-0001kk-0o; Fri, 25 Aug 2017 22:42:32 +0200"],"Date":"Fri, 25 Aug 2017 22:43:41 +0200 (CEST)","From":"Thomas Gleixner ","To":"Will Deacon ","cc":"Jiri Slaby , mingo@redhat.com,\n\tdvhart@infradead.org, peterz@infradead.org,\n\tlinux-kernel@vger.kernel.org, Richard Henderson ,\n\tIvan Kokshaysky ,\n\tMatt Turner , Vineet Gupta ,\n\tCatalin Marinas ,\n\tRichard Kuo , Tony Luck ,\n\tFenghua Yu , Michal Simek ,\n\tRalf Baechle , Jonas Bonn ,\n\tStefan Kristiansson ,\n\tStafford Horne ,\n\t\"James E.J. Bottomley\" ,\n\tHelge Deller ,\n\tBenjamin Herrenschmidt ,\n\tPaul Mackerras ,\n\tMartin Schwidefsky ,\n\tYoshinori Sato ,\n\tRich Felker , \"David S. Miller\" ,\n\t\"H. Peter Anvin\" , Chris Zankel ,\n\tMax Filippov ,\n\tArnd Bergmann , x86@kernel.org,\n\tlinux-alpha@vger.kernel.org, linux-snps-arc@lists.infradead.org,\n\tlinux-arm-kernel@lists.infradead.org,\n\tlinux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org,\n\tlinux-mips@linux-mips.org, openrisc@lists.librecores.org,\n\tlinux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,\n\tlinux-s390@vger.kernel.org, linux-sh@vger.kernel.org,\n\tsparclinux@vger.kernel.org, linux-xtensa@linux-xtensa.org,\n\tlinux-arch@vger.kernel.org","Subject":"Re: [PATCH v2 1/1] futex: remove duplicated code and fix UB","In-Reply-To":"<20170824094756.GA6346@arm.com>","Message-ID":"","References":"<20170824073105.3901-1-jslaby@suse.cz>\n\t<20170824094756.GA6346@arm.com>","User-Agent":"Alpine 2.20 (DEB 67 2015-01-07)","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","X-Linutronix-Spam-Score":"-1.0","X-Linutronix-Spam-Level":"-","X-Linutronix-Spam-Status":"No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,\n\tSHORTCIRCUIT=-0.0001","Sender":"sparclinux-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"sparclinux@vger.kernel.org"}}]