linux-kernel - Re: [PATCH v2] x86: fix early boot crash on gcc-10

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: <20200415074842.GA31016@zn.tnic>
Date:   Wed, 15 Apr 2020 09:48:42 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Michael Matz <matz@...e.de>
Cc:     Jakub Jelinek <jakub@...hat.com>,
        Sergei Trofimovich <slyfox@...too.org>,
        linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, x86@...nel.org
Subject: Re: [PATCH v2] x86: fix early boot crash on gcc-10

On Tue, Apr 14, 2020 at 01:50:29PM +0000, Michael Matz wrote:
> So this part expects that the caller (!) of trace_hardirqs_on was compiled
> with a frame pointer (in %ebp).

/me looks at the .s file...

options passed comment at the top has -fno-omit-frame-pointer

> Obviously that's not the case as you traced above. Is start_secondary
> the immediate caller in the above case?

Yes, start_secondary() is the function which is marked as
__attribute__((optimize("-fno-stack-protector"))) and it does:

# arch/x86/kernel/smpboot.c:264:        local_irq_enable();
        call    trace_hardirqs_on       #

(the local_irq_enable() is a macro which has the call to
trace_hardirqs_on().

> Look at it's disassembly.  If it doesn't have the usual push
> %ebp/mov%esp,%ebp prologue it probably doesn't use a frame pointer.

Here's the preamble:

        .text
        .p2align 4
        .type   start_secondary, @function
start_secondary:
        pushl   %esi    #
        pushl   %ebx    #
        subl    $28, %esp       #,
# arch/x86/kernel/smpboot.c:226:        cr4_init();
        call    cr4_init        #
	...

Those two pushes on entry are for callee-saved regs, AFAICT, because it
is going to use them later. But no frame setup.

> In that case I would speculate that having a frame pointer
> was (before the change above) only a side-effect of being
> compiled with -fstack-protector, which got now disabled.

Looks like it. And that's 32-bit. 64-bit variant of this looks more like it:

        .text
        .p2align 4
        .type   start_secondary, @function
start_secondary:
        pushq   %rbp    #
        subq    $8, %rsp        #,

although it doesn't save %rsp on the stack. I think it leaves space on
the stack for the canary but I'm not sure.

> But I was under the impression that the upstream kernels build with
> -fno-omit-frame-pointer, so that sounds unexpected.

Yap, see above.

> But I have no better explanation at the moment. If the above
> speculation doesn't make you progress: preprocessed file and a note of
> what the immediate caller of trace_hardirqs_on is in the above case,
> please.

Ok, I'll find you on IRC to talk details.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette