Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAKwvOd=jmi_7LvZbS0PA5-zt62m5ZFPUNG7JtMQVmerCR63ebQ@mail.gmail.com>
 <BDEF22F0-84A5-4A36-B153-13E6A4260922@zytor.com> <CAKwvOdnLt5qr_5ah86qdsocTxUbo_3X1y3PPRp1EZwt=__NxnA@mail.gmail.com>
 <dd1626ae-114f-3817-c8b2-f845e079bfc6@zytor.com> <CAKwvOdkEHqv7Ut8H6Rb+gSvfWsMHqO=E3f7yxm=06m9yEn=h6w@mail.gmail.com>
 <26B017D5-4063-46CB-8768-B0E5E7CD3838@zytor.com>
In-Reply-To: <26B017D5-4063-46CB-8768-B0E5E7CD3838@zytor.com>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Fri, 25 May 2018 09:27:40 -0700
Message-ID: <CAKwvOdk_4g5PsRUoLgOeWEwt+d0diyD_Ycx6s7LQsJrSMsBEuA@mail.gmail.com>
Subject: Re: [clang] stack protector and f1f029c7bf
To:     hpa@zytor.com
Cc:     Alistair Strachan <astrachan@google.com>,
        Manoj Gupta <manojgupta@google.com>,
        Matthias Kaehlcke <mka@google.com>,
        Greg Hackmann <ghackmann@google.com>, sedat.dilek@gmail.com,
        tstellar@redhat.com, LKML <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 24, 2018 at 3:43 PM <hpa@zytor.com> wrote:
> On May 24, 2018 3:31:05 PM PDT, Nick Desaulniers <ndesaulniers@google.com>
wrote:
> >On Thu, May 24, 2018 at 3:05 PM H. Peter Anvin <hpa@zytor.com> wrote:
> >> COMPILER AR: "=rm" should NEVER generate worse code than "=r". That
> >is
> >> unequivocally a compiler bug.
> >
> >Filed: https://bugs.llvm.org/show_bug.cgi?id=37583
> >
> >> >> You are claiming it doesn't buy us anything, but you are only
> >looking
> >at
> >> > the paravirt case which is kind of "special" (in the short bus kind
> >of
> >way),
> >> >
> >> > That's fair.  Is another possible solution to have paravirt maybe
> >not
> >use
> >> > native_save_fl() then, but its own
> >non-static-inline-without-m-constraint
> >> > implementation?
> >
> >> KERNEL AR: change native_save_fl() to an extern inline with an
> >assembly
> >> out-of-line implementation, to satisfy the paravirt requirement that
> >no
> >> GPRs other than %rax are clobbered.
> >
> >i'm happy to add that, do you have a recommendation if it should go in
> >an
> >existing .S file or a new one (and if so where/what shall I call it?).

> How about irqflags.c since that is what the .h file is called.

> It should simply be:

> push %rdi
> popf
> ret

> pushf
> pop %rax
> ret

> ... but with all the regular assembly decorations, of course.

Something like the following?


diff --git a/arch/x86/kernel/irqflags.c b/arch/x86/kernel/irqflags.c
new file mode 100644
index 000000000000..59dc21bd3327
--- /dev/null
+++ b/arch/x86/kernel/irqflags.c
@@ -0,0 +1,24 @@
+#include <asm/asm.h>
+
+extern unsigned long native_save_fl(void);
+extern void native_restore_fl(unsigned long flags);
+
+asm(
+".pushsection .text;"
+".global native_save_fl;"
+".type native_save_fl, @function;"
+"native_save_fl:"
+"pushf;"
+"pop %" _ASM_AX ";"
+"ret;"
+".popsection");
+
+asm(
+".pushsection .text;"
+".global native_restore_fl;"
+".type native_restore_fl, @function;"
+"native_restore_fl:"
+"push %" _ASM_DI ";"
+"popf;"
+"ret;"
+".popsection");

And change the declaration in arch/x86/include/asm/irqflags.h to:
+extern inline unsigned long native_save_fl(void);
+extern inline void native_restore_fl(unsigned long flags);

This seems to work, but
1. arch/x86/boot/compressed/misc.o warns that native_save_fl() is never
defined (arch_local_save_flags() uses it).  Does that mean
arch_local_save_flags(), and friends would also have to move to the newly
created .c file as well?
2. `extern inline` doesn't inline any instances (from what I can tell from
disassembling vmlinux).  I think this is strictly worse. Don't we only want
pv_irq_ops.save_fl to be non-inlined in a way that no stack protector can
be added? If that's the case, should my assembly based implementation have
a different identifier (`native_save_fl_paravirt` or something). That would
also fix point #1 above. But now the paravirt code has its own copy of the
function.
-- 
Thanks,
~Nick Desaulniers