linux-kernel - Re: [PATCH v4 1/7] x86/kexec: Consolidate relocate

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: <7eb254a7-473a-94c6-8dd5-24377ed67a34@amd.com>
Date: Mon, 21 Jul 2025 16:27:40 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: "Huang, Kai" <kai.huang@...el.com>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "peterz@...radead.org" <peterz@...radead.org>,
 "Hansen, Dave" <dave.hansen@...el.com>, "mingo@...hat.com"
 <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "hpa@...or.com" <hpa@...or.com>
Cc: "ashish.kalra@....com" <ashish.kalra@....com>,
 "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
 "seanjc@...gle.com" <seanjc@...gle.com>, "x86@...nel.org" <x86@...nel.org>,
 "kas@...nel.org" <kas@...nel.org>, "Gao, Chao" <chao.gao@...el.com>,
 "Chatre, Reinette" <reinette.chatre@...el.com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "Williams, Dan J" <dan.j.williams@...el.com>,
 "sagis@...gle.com" <sagis@...gle.com>,
 "pbonzini@...hat.com" <pbonzini@...hat.com>,
 "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "dwmw@...zon.co.uk" <dwmw@...zon.co.uk>,
 "Yamahata, Isaku" <isaku.yamahata@...el.com>,
 "nik.borisov@...e.com" <nik.borisov@...e.com>
Subject: Re: [PATCH v4 1/7] x86/kexec: Consolidate relocate_kernel() function
 parameters

On 7/21/25 16:20, Huang, Kai wrote:
> On Mon, 2025-07-21 at 09:40 -0500, Tom Lendacky wrote:
>> On 7/17/25 16:46, Kai Huang wrote:
>>> During kexec, the kernel jumps to the new kernel in relocate_kernel(),
>>> which is implemented in assembly and both 32-bit and 64-bit have their
>>> own version.
>>>
>>> Currently, for both 32-bit and 64-bit, the last two parameters of the
>>> relocate_kernel() are both 'unsigned int' but actually they only convey
>>> a boolean, i.e., one bit information.  The 'unsigned int' has enough
>>> space to carry two bits information therefore there's no need to pass
>>> the two booleans in two separate 'unsigned int'.
>>>
>>> Consolidate the last two function parameters of relocate_kernel() into a
>>> single 'unsigned int' and pass flags instead.
>>>
>>> Only consolidate the 64-bit version albeit the similar optimization can
>>> be done for the 32-bit version too.  Don't bother changing the 32-bit
>>> version while it is working (since assembly code change is required).
>>>
>>> Signed-off-by: Kai Huang <kai.huang@...el.com>
>>> ---
>>>  arch/x86/include/asm/kexec.h         | 12 ++++++++++--
>>>  arch/x86/kernel/machine_kexec_64.c   | 22 +++++++++++++---------
>>>  arch/x86/kernel/relocate_kernel_64.S | 19 +++++++++----------
>>>  3 files changed, 32 insertions(+), 21 deletions(-)
>>>

>>> @@ -204,7 +202,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>>>  	 * entries that will conflict with the now unencrypted memory
>>>  	 * used by kexec. Flush the caches before copying the kernel.
>>>  	 */
>>> -	testq	%r8, %r8
>>> +	testq	$RELOC_KERNEL_HOST_MEM_ACTIVE, %r11
>>
>> Hmmm... can't both bits be set at the same time? If so, then this will
>> fail. This should be doing bit tests now.
> 
> TEST instruction performs logical AND of the two operands, therefore the
> above equals to:
> 
> 	set ZF if "R11 AND BIT(1) == 0".
> 
> Whether there's any other bits set in R11 doesn't impact the above, right?
>  

Doh! My bad, yes, not sure what I was thinking there.

Thanks,
Tom

>>
>>>  	jz .Lsme_off
>>>  	wbinvd
>>>  .Lsme_off:
>>> @@ -220,7 +218,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>>>  	movq	%cr3, %rax
>>>  	movq	%rax, %cr3
>>>  
>>> -	testq	%r11, %r11	/* preserve_context */
>>> +	testq	$RELOC_KERNEL_PRESERVE_CONTEXT, %r11
>>>  	jnz .Lrelocate
>>>  
>>>  	/*
>>> @@ -273,7 +271,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
>>>  	ANNOTATE_NOENDBR
>>>  	andq	$PAGE_MASK, %r8
>>>  	lea	PAGE_SIZE(%r8), %rsp
>>> -	movl	$1, %r11d	/* Ensure preserve_context flag is set */
>>> +	movl	$RELOC_KERNEL_PRESERVE_CONTEXT, %r11d	/* Ensure preserve_context flag is set */
>>
>> And this will clear any value that was in r11 vs setting a single bit.
>> Not sure it currently has any effect because r8 (where the memory
>> encryption setting was held) is modified just before this. But if any
>> bits are added in the future that are needed past here, this will be a
>> problem.
> 
> Right.  It's just for the
> 
> 	call swap_pages
> 
> right after it.  Nothing else later uses RELOC_KERNEL_PRESERVE_CONTEXT or
> RELOC_KERNEL_HOST_MEM_ACTIVE.
> 
> Maybe we can add a comment to remind that all other flags are not restored
> so if someone wants to add a new bit and use it at a later he/she can see?
> 
> 	/*
> 	 * Ensure RELOC_KERNEL_PRESERVE_CONTEXT flag is set so swap_pages
> 	 * can do things correctly.  Note this doesn't restore any other 
> 	 * RELOC_KERNEL_* flags that were passed to relocate_kernel().
> 	 */
>>
>>>  	call	swap_pages
>>>  	movq	kexec_va_control_page(%rip), %rax
>>>  0:	addq	$virtual_mapped - 0b, %rax
>>> @@ -321,7 +319,7 @@ SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
>>>  	UNWIND_HINT_END_OF_STACK
>>>  	/*
>>>  	 * %rdi indirection page
>>> -	 * %r11 preserve_context
>>> +	 * %r11 flags: RELOC_KERNEL_*
>>>  	 */
>>>  	movq	%rdi, %rcx	/* Put the indirection_page in %rcx */
>>>  	xorl	%edi, %edi
>>> @@ -357,7 +355,8 @@ SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
>>>  	movq	%rdi, %rdx    /* Save destination page to %rdx */
>>>  	movq	%rsi, %rax    /* Save source page to %rax */
>>>  
>>> -	testq	%r11, %r11    /* Only actually swap for ::preserve_context */
>>> +	/* Only actually swap for ::preserve_context */
>>> +	testq	$RELOC_KERNEL_PRESERVE_CONTEXT, %r11
>>
>> Ditto here on the bit testing.
> 
> I don't see any problem?  Please see above.