From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754090AbdEIRoJ (ORCPT ); Tue, 9 May 2017 13:44:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52594 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751306AbdEIRoH (ORCPT ); Tue, 9 May 2017 13:44:07 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EDA8A7F4BB Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com EDA8A7F4BB Date: Tue, 9 May 2017 12:44:03 -0500 From: Josh Poimboeuf To: Kees Cook Cc: LKML , Peter Zijlstra , PaX Team , Jann Horn , Eric Biggers , Christoph Hellwig , "axboe@kernel.dk" , James Bottomley , Elena Reshetova , Hans Liljestrand , David Windsor , "x86@kernel.org" , Ingo Molnar , Arnd Bergmann , Greg Kroah-Hartman , "David S. Miller" , Rik van Riel , linux-arch , "kernel-hardening@lists.openwall.com" Subject: Re: [PATCH v3 2/2] x86/refcount: Implement fast refcount overflow protection Message-ID: <20170509174403.etx64yozb7ctr4ur@treble> References: <1494271972-140319-1-git-send-email-keescook@chromium.org> <1494271972-140319-3-git-send-email-keescook@chromium.org> <20170508225308.a6uznrhdm7pgyhcg@treble> <20170509015829.iycvxxbyblfgklsg@treble> <20170509170812.uyfkvblwofhxpk4e@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 09 May 2017 17:44:07 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 09, 2017 at 10:29:16AM -0700, Kees Cook wrote: > On Tue, May 9, 2017 at 10:08 AM, Josh Poimboeuf wrote: > > On Mon, May 08, 2017 at 08:58:29PM -0500, Josh Poimboeuf wrote: > >> On Mon, May 08, 2017 at 04:31:11PM -0700, Kees Cook wrote: > >> > On Mon, May 8, 2017 at 3:53 PM, Josh Poimboeuf wrote: > >> > > On Mon, May 08, 2017 at 12:32:52PM -0700, Kees Cook wrote: > >> > >> +#define REFCOUNT_EXCEPTION \ > >> > >> + "movl $0x7fffffff, %[counter]\n\t" \ > >> > >> + "int $"__stringify(X86_REFCOUNT_VECTOR)"\n" \ > >> > >> + "0:\n\t" \ > >> > >> + _ASM_EXTABLE(0b, 0b) > >> > > > >> > > Despite the objtool warnings going away, this still uses the exception > >> > > table in a new way, which will confuse objtool. I need to do some more > >> > > thinking about the best way to fix it, either as a change to your patch > >> > > or a change to objtool. > >> > > >> > In that it's not a "true" exception? > >> > >> Right. And also that it doesn't need the "fixup" since it would return > >> to the same address anyway. > > > > How about the following on top of your patch? It uses #UD (invalid > > opcode). Notice it's mostly code deletions :-) > > Hah, I wrote this patch almost exactly last night, but hadn't had a > chance to send it out. :) > > I ended up defining a new exception handler, which means nothing > special in the generic trap code. I didn't send it out because it was > still using a jns instead of js, and I was pondering if I wanted to > reintroduce the text section jump just to gain the initial benefit of > forward-branch-not-taken optimization... > > > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > > index 6e8bbd7..653a985 100644 > > --- a/arch/x86/include/asm/refcount.h > > +++ b/arch/x86/include/asm/refcount.h > > @@ -8,15 +8,16 @@ > > */ > > #include > > #include > > +#include > > > > #define REFCOUNT_EXCEPTION \ > > "movl $0x7fffffff, %[counter]\n\t" \ > > - "int $"__stringify(X86_REFCOUNT_VECTOR)"\n" \ > > - "0:\n\t" \ > > - _ASM_EXTABLE(0b, 0b) > > + "1:\t" ASM_UD0 "\n" \ > > + "2:\n\t" \ > > + _ASM_EXTABLE(1b, 2b) > > I used _ASM_EXTABLE_REFCOUNT(1b, 2b) here, with > arch/x86/include/asm/asm.h adding: > > +# define _ASM_EXTABLE_REFCOUNT(from, to) \ > + _ASM_EXTABLE_HANDLE(from, to, ex_handler_refcount) > > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > > index 0b2dbcc..7de95b7 100644 > > --- a/arch/x86/kernel/traps.c > > +++ b/arch/x86/kernel/traps.c > > @@ -220,8 +220,8 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str, > > if (!user_mode(regs)) { > > if (fixup_exception(regs, trapnr)) { > > if (IS_ENABLED(CONFIG_FAST_REFCOUNT) && > > - trapnr == X86_REFCOUNT_VECTOR) > > - refcount_error_report(regs, str); > > + trapnr == X86_TRAP_UD) > > + refcount_error_report(regs); > > > > return 0; > > } > > And then I could leave out this hunk, instead adding this to > arch/x86/mm/extable.c: > > +bool ex_handler_refcount(const struct exception_table_entry *fixup, > + struct pt_regs *regs, int trapnr) > +{ > + regs->ip = ex_fixup_addr(fixup); > + refcount_error_report(regs, "overflow"); > + return true; > +} > +EXPORT_SYMBOL_GPL(ex_handler_refcount); > > After looking at the assembly output, the "movl" instructions can be > various sizes, depending on where %[counter] lives, so I'm also > considering returning to using PaX's "lea", but I'm not sure the > benefit would be very large. Good, your patch sounds better than mine :-) -- Josh