linux-kernel - Re: [PATCH] x86/head/64: Switch to KERNEL

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: <57e7b3df-5ca3-b400-d99a-a430bcc35a7a@intel.com>
Date:   Thu, 25 May 2023 16:17:55 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Tom Lendacky <thomas.lendacky@....com>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Michael Roth <michael.roth@....com>,
        Ashish Kalra <ashish.kalra@....com>,
        Joerg Roedel <jroedel@...e.de>
Subject: Re: [PATCH] x86/head/64: Switch to KERNEL_CS as soon as new GDT is
 installed

On 5/17/23 09:26, Tom Lendacky wrote:
> However, a recent patchset that looked to avoid using the legacy
> decompressor during an EFI boot exposed this bug. At entry to startup_64,
> the CS value is that of EFI and is not mapped in the new kernel GDT. So
> when a #VC exception occurs, the CS value used by IRETQ is not valid and
> the guest boot crashes.

This confused me a bit.  Nobody merged that patchset yet, right?  You
just happened across this issue when debugging a crash in that *other* set?

> Fix this issue by moving the block that switches to the KERNEL_CS value to
> be done immediately after returning from startup_64_setup_env().
> 
> Fixes: bcce82908333 ("x86/sev: Detect/setup SEV/SME features earlier in boot")
> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>

Any thoughts on whether we want this in stable@?

I also wonder whether we need a comment in that little chunk of code
something along the lines of:

	/*
	 * Do not add anything which might take a fault or exception.
	 * IRET does not work here.
	 */

Michael, do you think you would have spotted something like this had it
been in the code when you were patching it?