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: <1726d92e-2574-40dc-8991-eed0184f957e@amd.com>
Date:   Mon, 4 Dec 2023 10:06:42 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Borislav Petkov <bp@...en8.de>, X86 ML <x86@...nel.org>
Cc:     Joerg Roedel <joro@...tes.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/sev: Do the C-bit verification only on the BSP

On 11/30/23 07:26, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@...en8.de>
> 
> There's no need to do it on every AP.
> 
> The C-bit value read on the BSP and also verified there, is used
> everywhere from now on.
> 
> There should be no functional changes resulting from this patch - just
> a bit faster booting APs.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@...en8.de>

One minor question below, but otherwise

Acked-by: Tom Lendacky <thomas.lendacky@....com>

> ---
>   arch/x86/kernel/head_64.S | 31 ++++++++++++++++++++++---------
>   1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> index 3dcabbc49149..af40d8eb4dca 100644
> --- a/arch/x86/kernel/head_64.S
> +++ b/arch/x86/kernel/head_64.S
> @@ -114,6 +114,28 @@ SYM_CODE_START_NOALIGN(startup_64)
>   
>   	/* Form the CR3 value being sure to include the CR3 modifier */
>   	addq	$(early_top_pgt - __START_KERNEL_map), %rax
> +
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +	mov	%rax, %rdi
> +	mov	%rax, %r14
> +
> +	addq	phys_base(%rip), %rdi
> +
> +	/*
> +	 * For SEV guests: Verify that the C-bit is correct. A malicious
> +	 * hypervisor could lie about the C-bit position to perform a ROP
> +	 * attack on the guest by writing to the unencrypted stack and wait for
> +	 * the next RET instruction.
> +	 */
> +	call	sev_verify_cbit
> +
> +	/*
> +	 * Restore CR3 value without the phys_base which will be added
> +	 * below, before writing %cr3.
> +	 */
> +	 mov	%r14, %rax

You're ignoring RAX now on return, so you can probably just make 
sev_verify_cbit() a void function now. You would still need to save RAX 
because of the calling convention, though, so it doesn't make this code 
any cleaner (other than the comment could then just say restore CR3 
value). You're call, I'm good either way.

Thanks,
Tom

> +#endif
> +
>   	jmp 1f
>   SYM_CODE_END(startup_64)
>   
> @@ -192,15 +214,6 @@ SYM_INNER_LABEL(secondary_startup_64_no_verify, SYM_L_GLOBAL)
>   	/* Setup early boot stage 4-/5-level pagetables. */
>   	addq	phys_base(%rip), %rax
>   
> -	/*
> -	 * For SEV guests: Verify that the C-bit is correct. A malicious
> -	 * hypervisor could lie about the C-bit position to perform a ROP
> -	 * attack on the guest by writing to the unencrypted stack and wait for
> -	 * the next RET instruction.
> -	 */
> -	movq	%rax, %rdi
> -	call	sev_verify_cbit
> -
>   	/*
>   	 * Switch to new page-table
>   	 *

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ