linux-kernel - Re: [PATCH 8/9] x86/dumpstack: Save first regs set for the executive summary

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: <CA+55aFwmi38uJyir_e_TTDObBGg55WsTB_0AqcVHzgU=+Fy=ag@mail.gmail.com>
Date:   Fri, 16 Mar 2018 10:22:29 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>, X86 ML <x86@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 8/9] x86/dumpstack: Save first regs set for the executive summary

On Fri, Mar 16, 2018 at 4:48 AM, Borislav Petkov <bp@...en8.de> wrote:
> On Thu, Mar 15, 2018 at 02:01:32PM -0500, Josh Poimboeuf wrote:
>> no_context() has the following line, right before it calls oops_end():
>>
>>       /* Executive summary in case the body of the oops scrolled away */
>>       printk(KERN_DEFAULT "CR2: %016lx\n", address);
>>
>> I think that line can now be removed, since the executive summary
>> __show_regs() will include CR2.
>
> Good idea. Done.

NOOOO!

Guys, %cr2 CAN AND DOES CHANGE!

The reason we do that

        printk(KERN_DEFAULT "CR2: %016lx\n", address);

is because WE ARE NOT PRINTING OUT THE CURRENT CR2 REGISTER!

This is really damn important.

The "address" register contains the CR2 value as it was read *very*
early in the page fault case, before we enabled interrupts, and before
we did various random things that can cause further page faults and
change CR2!

So the executive summary that does __show_regs() may end up showing
something completely different than the actual faulting address,
because we might have taken a vmalloc-space exception in the meantime,
for example.

Do *NOT* get rid of that thing.

You're better off getting rid of the CR2 line from __show_regs(),
because it can be dangerously confusing. It's not actually part of the
saved register state at all, it's something entirely different. It's
like showing the current eflags rather than the eflags saved on the
faulting stack.

                     Linus