Re: [PATCH] x86/boot: Fix kexec booting failure after SEV early boot support

From: Borislav Petkov <bp@suse.de>
To: "Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
	Kairui Song <kasong@redhat.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"Singh, Brijesh" <brijesh.singh@amd.com>,
	"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
	"dyoung@redhat.com" <dyoung@redhat.com>
Subject: Re: [PATCH] x86/boot: Fix kexec booting failure after SEV early boot support
Date: Tue, 25 Sep 2018 19:26:08 +0200	[thread overview]
Message-ID: <20180925172608.GB15464@zn.tnic> (raw)
In-Reply-To: <6e15796e-31e9-2dc6-4a31-5c1b01554b45@amd.com>

On Tue, Sep 25, 2018 at 02:33:48PM +0000, Lendacky, Thomas wrote:
> On 09/25/2018 06:10 AM, Kairui Song wrote:
> > Commit 1958b5fc4010 ("x86/boot: Add early boot support when running
> > with SEV active") is causing kexec becomes sometimes unstable, kexec
> > reboot won't start a second kernel bypassing BIOS boot process, instead,
> > the system got reset.
> > 
> > That's because, in get_sev_encryption_bit function, we are using
> > 32-bit RIP-relative addressing to read the value of enc_bit, but
> > kexec may alloc the early boot up code to a higher location, which
> > is beyond 32-bit addressing limit. Some garbage will be read and
> > get_sev_encryption_bit will return the wrong value, which lead to
> > wrong memory page flag.
> > 
> > This patch adds a get_sev_encryption_bit_64 function to avoid this
> > problem. 64-bit early boot code will use this function instead, it
> > uses native RIP addressing to read the enc_bit which have no problem
> > with any location.
> > 
> > Fixes: 1958b5fc4010 ("x86/boot: Add early boot support when running with SEV active")
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> > ---
> >  arch/x86/boot/compressed/mem_encrypt.S | 64 ++++++++++++++++++--------
> >  1 file changed, 45 insertions(+), 19 deletions(-)
> > 
> > diff --git a/arch/x86/boot/compressed/mem_encrypt.S b/arch/x86/boot/compressed/mem_encrypt.S
> > index eaa843a52907..41933550449a 100644
> > --- a/arch/x86/boot/compressed/mem_encrypt.S
> > +++ b/arch/x86/boot/compressed/mem_encrypt.S
> > @@ -18,27 +18,13 @@
> >  
> >  	.text
> >  	.code32
> > -ENTRY(get_sev_encryption_bit)
> > +do_get_sev_encryption_bit:
> >  	xor	%eax, %eax
> >  
> >  #ifdef CONFIG_AMD_MEM_ENCRYPT
> >  	push	%ebx
> >  	push	%ecx
> >  	push	%edx
> > -	push	%edi
> > -
> > -	/*
> > -	 * RIP-relative addressing is needed to access the encryption bit
> > -	 * variable. Since we are running in 32-bit mode we need this call/pop
> > -	 * sequence to get the proper relative addressing.
> > -	 */
> > -	call	1f
> > -1:	popl	%edi
> > -	subl	$1b, %edi
> > -
> > -	movl	enc_bit(%edi), %eax
> > -	cmpl	$0, %eax
> > -	jge	.Lsev_exit
> >  
> >  	/* Check if running under a hypervisor */
> >  	movl	$1, %eax
> > @@ -69,25 +55,65 @@ ENTRY(get_sev_encryption_bit)
> >  
> >  	movl	%ebx, %eax
> >  	andl	$0x3f, %eax		/* Return the encryption bit location */
> > -	movl	%eax, enc_bit(%edi)

IINM, the problem can be addressed in a simpler way by getting rid of
enc_bit and thus getting rid of the need to do relative addressing of
anything and simply doing the whole dance of figuring out the C-bit each
time. It probably wouldn't be even measurable...

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 