Re: [PATCH v5 13/20] x86/kexec: Mark relocate_kernel page as ROX instead of RWX

[Nathan Chancellor <nathan@kernel.org>]

On Thu, Dec 12, 2024 at 05:00:00PM +0000, David Woodhouse wrote:
> > 
> > Here is the output that I see with that patch applied when rebooting via
> > 'systemctl kexec':
> > 
> > RAX=000000010c070001 RBX=0000000000000000 RCX=0000000000000000 RDX=000000047fffb1a0
> > RSI=000000011c444000 RDI=000000011c450002 RBP=ff1cd0424d6e8c00 RSP=ff4178d5c5aebc60
> > R8 =0000000000000000 R9 =000000011c446000 R10=ffffffff909f3e00 R11=0000000000000003
> > R12=0000000000000000 R13=0000000000000001 R14=00000000fee1dead R15=0000000000000000
> > RIP=ff1cd0425c44401c RFL=00010002 [-------] 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 fffffe043fedb000 00004087 00008b00 DPL=0 TSS64-busy
> > GDT=     0000000000000000 00000000
> > IDT=     0000000000000000 00000000
> > CR0=80050033 CR2=ff1cd0425c4441de CR3=000000011c446000 CR4=00771ef0
> > DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000
> > DR6=00000000fffe0ff0 DR7=0000000000000400
> > EFER=0000000000000d01
> > Code=41 57 9c 6a 00 9d 0f 20 d8 4c 8b 0d ee 01 00 00 41 0f 22 d9 <48> 89 25 bb 01 00 00 48 89 05 c4 01 00 00 0f 20 c0 48 89 05 b2 01 00 00 41 0f 20 e5 4c 89
> 
> ...
> 
> > I have attached the output of 'objdump -S'. Please let me know if you
> > would like any other information or testing.
> 
> 	/* Switch to the identity mapped page tables */
> 	movq	%cr3, %rax
>    e:	0f 20 d8             	mov    %cr3,%rax
> 	movq	kexec_pa_table_page(%rip), %r9
>   11:	4c 8b 0d 00 00 00 00 	mov    0x0(%rip),%r9        # 18 <relocate_kernel+0x18>
> 	movq	%r9, %cr3
>   18:	41 0f 22 d9          	mov    %r9,%cr3
> 
> 	/* Save %rsp and CRs. */
> 	movq    %rsp, saved_rsp(%rip)
>   1c:	48 89 25 00 00 00 00 	mov    %rsp,0x0(%rip)        # 23 <relocate_kernel+0x23>
> 	movq	%rax, saved_cr3(%rip)
> 
> 
> So it's faulting when it tries to write to saved_rsp, which is the
> first instruction after it loads the new page tables into %cr3.
> 
> Before loading %cr3, the CPU is running on the original kernel page
> tables, It's running from the *virtual* address of the control page,
> which in your case is ff1cd0425c444000. In those old page tables, this
> control page is read-only.
> 
> But in the newly-loaded page tables, this control page should be read-
> write. 
> 
> What CPU is this? Shouldn't loading %cr3 have caused the existing TLB
> entry to be flushed?

The machine I have mainly been reproducing and testing this issue on is
an Intel Xeon Gold 6314U but I initially noticed this problem on a few
consumer AMD and Intel chips that I have locally.

> Can you tell me what happens if we don't write through *that* virtual
> mapping of the page, but instead do it a few instructions later through
> the identity mapping of the same page (which surely *won't* have an
> older, non-writeable TLB entry already...)?

Applying that diff on top of 5a82223e0743 makes the issue go away for
me.

> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 8bc86a1e056a..2763c509e513 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -70,6 +70,18 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
>  	movq	kexec_pa_table_page(%rip), %r9
>  	movq	%r9, %cr3
>  
> +	/* setup a new stack at the end of the physical control page */
> +	lea	PAGE_SIZE(%rsi), %rsp
> +
> +	/* jump to identity mapped page */
> +	addq	$(identity_mapped - relocate_kernel), %rsi
> +	ANNOTATE_RETPOLINE_SAFE
> +	jmp	*%rsi
> +	int3
> +SYM_CODE_END(relocate_kernel)
> +
> +SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> +	UNWIND_HINT_END_OF_STACK
>  	/* Save %rsp and CRs. */
>  	movq    %rsp, saved_rsp(%rip)
>  	movq	%rax, saved_cr3(%rip)
> @@ -85,19 +97,6 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
>  	/* Save the preserve_context to %r11 as swap_pages clobbers %rcx. */
>  	movq	%rcx, %r11
>  
> -	/* setup a new stack at the end of the physical control page */
> -	lea	PAGE_SIZE(%rsi), %rsp
> -
> -	/* jump to identity mapped page */
> -	addq	$(identity_mapped - relocate_kernel), %rsi
> -	pushq	%rsi
> -	ANNOTATE_UNRET_SAFE
> -	ret
> -	int3
> -SYM_CODE_END(relocate_kernel)
> -
> -SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> -	UNWIND_HINT_END_OF_STACK
>  	/*
>  	 * %rdi	indirection page
>  	 * %rdx start address
>