linux-kernel - Re: PROBLEM: consolidated IDT invalidation causes kexec to reboot

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171225212934.GA1410@arch-chirva.localdomain>
Date:   Mon, 25 Dec 2017 16:29:34 -0500
From:   Alexandru Chirvasitu <achirvasub@...il.com>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        kernel list <linux-kernel@...r.kernel.org>,
        Borislav Petkov <bp@...en8.de>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: PROBLEM: consolidated IDT invalidation causes kexec to reboot

Thanks for that!

On Mon, Dec 25, 2017 at 06:40:14AM -0800, Andy Lutomirski wrote:
> On Sat, Dec 23, 2017 at 7:30 PM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> > On Sat, Dec 23, 2017 at 5:44 PM, Alexandru Chirvasitu
> > <achirvasub@...il.com> wrote:
> >>
> >> For testing purposes, I've altered machine_kexec_32.c making the
> >> following toy commit. It naively undoes part of e802a51, solely to
> >> confirm that's where it goes awry in my setup.
> >
> > That's really funky.
> >
> > The idt_invalidate() seems to do *exactly* the same thing. It uses
> > "load_idt()" on an IDT with size 0, and the supplied address.
> >
> > Can you disassemble your "set_idt()" code vs the "idt_invalidate()"?
> >
> >> Is this expected behaviour?
> >
> > No. The code literally seems identical. The only difference is
> >
> >  (a) where the 0 limit comes from
> >
> >  (b) perhaps build flags and whether it is inlined or not due to being
> > in a different file
> >
> > and neither of those should matter, but maybe they do.
> >
> > Which is why I'd like you to actually look at the generated code and
> > see if you can see any difference..
> >
> 
> This is presumably the same call-tracing-without-TLS-working problem.
> idt_invalidate() is out-of-line and is compiled with full tracing on,
> and we're calling it from a context without TLS working (it's
> explicitly disabled in load_segments()) in machine_kexec_32.c.  The
> right fix is probably to inline idt_invalidate() and to add a comment.
>

This works.. I went back to the troublesome commit e802a51 and
modified it as follows:

--------------------------------------------------------
    make idt_invalidate static inline in header file

diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index 33aff45..87ca363 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -504,6 +504,17 @@ static inline void load_current_idt(void)
                load_idt((const struct desc_ptr *)&idt_descr);
 }
 
-extern void idt_invalidate(void *addr);
+
+/**
+ * idt_invalidate - Invalidate interrupt descriptor table
+ * @addr:       The virtual address of the 'invalid' IDT
+ */
+static inline void idt_invalidate(void *addr)
+{
+        struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 };
+
+        load_idt(&idt);
+}
+
 
 #endif /* _ASM_X86_DESC_H */
diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c
index cd4658c..fec44c6 100644
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -25,13 +25,3 @@ const struct desc_ptr debug_idt_descr = {
 };
 #endif
 
-/**
- * idt_invalidate - Invalidate interrupt descriptor table
- * @addr:      The virtual address of the 'invalid' IDT
- */
-void idt_invalidate(void *addr)
-{
-       struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 };
-
-       load_idt(&idt);
-}
--------------------------------------------------------

kexec now works as expected; tested repeatedly, both with direct
execution and crash triggering.

I had to google 'inline function' :)). 

> Also, why idt_invalidate(phys_to_virt(0))?  That makes rather little
> sense to me.
> 
> --Andy