Re: [PATCH v5 07/20] x86/kexec: Invoke copy of relocate_kernel() instead of the original

[Nathan Chancellor <nathan@kernel.org>]

Hi David,

On Thu, Dec 05, 2024 at 03:05:13PM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This currently calls set_memory_x() from machine_kexec_prepare() just
> like the 32-bit version does. That's actually a bit earlier than I'd
> like, as it leaves the page RWX all the time the image is even *loaded*.
> 
> Subsequent commits will eliminate all the writes to the page between the
> point it's marked executable in machine_kexec_prepare() the time that
> relocate_kernel() is running and has switched to the identmap %cr3, so
> that it can be ROX. But that can't happen until it's moved to the .data
> section of the kernel, and *that* can't happen until we start executing
> the copy instead of executing it in place in the kernel .text. So break
> the circular dependency in those commits by letting it be RWX for now.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>  arch/x86/kernel/machine_kexec_64.c   | 30 ++++++++++++++++++++++------
>  arch/x86/kernel/relocate_kernel_64.S |  5 ++++-
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
> index 3a4cbac1a0c6..9567347f7a9b 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
...
>  void machine_kexec(struct kimage *image)
>  {
> +	unsigned long (*relocate_kernel_ptr)(unsigned long indirection_page,
> +					     unsigned long page_list,
> +					     unsigned long start_address,
> +					     unsigned int preserve_context,
> +					     unsigned int host_mem_enc_active);
>  	unsigned long page_list[PAGES_NR];
>  	unsigned int host_mem_enc_active;
>  	int save_ftrace_enabled;
> @@ -371,6 +387,8 @@ void machine_kexec(struct kimage *image)
>  		page_list[PA_SWAP_PAGE] = (page_to_pfn(image->swap_page)
>  						<< PAGE_SHIFT);
>  
> +	relocate_kernel_ptr = control_page;
> +

Because of this change...

>  	/*
>  	 * The segment registers are funny things, they have both a
>  	 * visible and an invisible part.  Whenever the visible part is
> @@ -390,11 +408,11 @@ void machine_kexec(struct kimage *image)
>  	native_gdt_invalidate();
>  
>  	/* now call it */
> -	image->start = relocate_kernel((unsigned long)image->head,
> -				       (unsigned long)page_list,
> -				       image->start,
> -				       image->preserve_context,
> -				       host_mem_enc_active);
> +	image->start = relocate_kernel_ptr((unsigned long)image->head,
> +					   (unsigned long)page_list,
> +					   image->start,
> +					   image->preserve_context,
> +					   host_mem_enc_active);

kexec-ing on a CONFIG_CFI_CLANG kernel crashes and burns:

RAX=0000000000000018 RBX=ff408cf982596000 RCX=0000000000000000 RDX=000000047fffb280
RSI=ff6c99a785943d20 RDI=000000011e145002 RBP=ff6c99a785943d70 RSP=ff6c99a785943d10
R8 =0000000000000000 R9 =0000000000000000 R10=00000000ab150dc6 R11=ff408cf99e139000
R12=0000000028121969 R13=00000000fee1dead R14=0000000000000000 R15=0000000000000001
RIP=ffffffff84c9c510 RFL=00010086 [--S--P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
CS =0010 0000000000000000 ffffffff 00a09b00 DPL=0 CS64 [-RA]
SS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
DS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
FS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
GS =0018 0000000000000000 ffffffff 00c09300 DPL=0 DS   [-WA]
LDT=0000 0000000000000000 ffffffff 00c00000
TR =0040 fffffe441f360000 00004087 00008b00 DPL=0 TSS64-busy
GDT=     0000000000000000 00000000
IDT=     0000000000000000 00000000
CR0=80050033 CR2=00007ffde71dbfc0 CR3=00000001140b0002 CR4=00771ef0
DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
DR6=00000000fffe0ff0 DR7=0000000000000400
EFER=0000000000000d01
Code=83 e1 01 48 8d 74 24 10 41 ba c6 0d 15 ab 45 03 53 f1 74 02 <0f> 0b 41 ff d3 0f 1f 00 48 89 43 18 f6 83 78 02 00 00 02 74 05 e8 16 06 da 00 44 89 3d 17

     a0c: 83 e1 01                      andl    $0x1, %ecx
     a0f: 48 8d 74 24 10                leaq    0x10(%rsp), %rsi
;   image->start = relocate_kernel_ptr((unsigned long)image->head,
     a14: 41 ba 67 a6 7c e6             movl    $0xe67ca667, %r10d      # imm = 0xE67CA667
     a1a: 45 03 53 f1                   addl    -0xf(%r11), %r10d
     a1e: 74 02                         je  0xa22 <machine_kexec+0x1c2>
     a20: 0f 0b                         ud2
     a22: 2e e8 00 00 00 00             callq   0xa28 <machine_kexec+0x1c8>
     a28: 48 89 43 18                   movq    %rax, 0x18(%rbx)

I guess this seems somewhat unavoidable because control_page is just a
'void *', perhaps machine_kexec() should just be marked as __nocfi? This
diff resolves that issue for me.

Cheers,
Nathan

diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c
index 9567347f7a9b..e77110c4bb91 100644
--- a/arch/x86/kernel/machine_kexec_64.c
+++ b/arch/x86/kernel/machine_kexec_64.c
@@ -334,7 +334,7 @@ void machine_kexec_cleanup(struct kimage *image)
  * Do not allocate memory (or fail in any way) in machine_kexec().
  * We are past the point of no return, committed to rebooting now.
  */
-void machine_kexec(struct kimage *image)
+void __nocfi machine_kexec(struct kimage *image)
 {
 	unsigned long (*relocate_kernel_ptr)(unsigned long indirection_page,
 					     unsigned long page_list,