| Message ID | 20230728090916.1538550-2-ardb@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi/x86: Avoid bare metal decompressor during EFI boot | expand |
On Fri, Jul 28, 2023 at 11:08:55AM +0200, Ard Biesheuvel wrote: > The 4-to-5 level mode switch trampoline disables long mode and paging in > order to be able to flick the LA57 bit. According to section 3.4.1.1 of > the x86 architecture manual [0], we should not rely on 64-bit GPRs Please use passive voice in your commit message: no "we" or "I", etc, and describe your changes in imperative mood. > retaining the upper 32 bits of their contents across such a mode switch. > > Given that RBP, RBX and RSI are live at this point, let's preserve them > on the stack, along with the return address that might be above 4G as > well. > > [0] Intel® 64 and IA-32 Architectures Software Developer’s Manual, Volume 1: Basic Architecture > > "Because the upper 32 bits of 64-bit general-purpose registers are > undefined in 32-bit modes, the upper 32 bits of any general-purpose > register are not preserved when switching from 64-bit mode to a 32-bit > mode (to protected mode or compatibility mode). Software must not > depend on these bits to maintain a value after a 64-bit to 32-bit > mode switch." > > Fixes: 194a9749c73d650c ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/boot/compressed/head_64.S | 23 +++++++++++++++----- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > index 03c4328a88cbd5d0..f7c11a0018477de8 100644 > --- a/arch/x86/boot/compressed/head_64.S > +++ b/arch/x86/boot/compressed/head_64.S > @@ -459,11 +459,22 @@ SYM_CODE_START(startup_64) > /* Save the trampoline address in RCX */ > movq %rax, %rcx > > + /* Set up 32-bit addressable stack */ > + leaq TRAMPOLINE_32BIT_STACK_END(%rcx), %rsp > + > /* > - * Load the address of trampoline_return() into RDI. > - * It will be used by the trampoline to return to the main code. > + * Load the address of trampoline_return() into RDI and push it onto > + * the stack so it will survive 32-bit truncation due to the 32-bit I think you should explain here what that SDM excerpt above says so that it is clear what "32-bit truncation" is meant. > + * protected mode switch. It will be used by the trampoline to return > + * to the main code. > */ > leaq trampoline_return(%rip), %rdi > + pushq %rdi > + > + /* Preserve other live 64-bit registers */ > + pushq %rbp > + pushq %rbx > + pushq %rsi > > /* Switch to compatibility mode (CS.L = 0 CS.D = 1) via far return */ > pushq $__KERNEL32_CS What is more interesting is why is this trampoline crap unconditional? /me goes and does git archeology... 194a9749c73d ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") We go through the trampoline even if we don't have to: if we're already in 5-level paging mode or if we don't need to switch to it. This way the trampoline gets tested on every boot. Well, f*ck no. All my machines don't have 5level pages. And I bet 5level pages is still not in the majority of the machines out there. This all needs to be abstracted away into a separate function which exits early if no 5 level support. Kirill, please fix this. Thx.
On Mon, 31 Jul 2023 at 12:07, Borislav Petkov <bp@alien8.de> wrote: > > On Fri, Jul 28, 2023 at 11:08:55AM +0200, Ard Biesheuvel wrote: > > The 4-to-5 level mode switch trampoline disables long mode and paging in > > order to be able to flick the LA57 bit. According to section 3.4.1.1 of > > the x86 architecture manual [0], we should not rely on 64-bit GPRs > > Please use passive voice in your commit message: no "we" or "I", etc, > and describe your changes in imperative mood. > Sure. > > retaining the upper 32 bits of their contents across such a mode switch. > > > > Given that RBP, RBX and RSI are live at this point, let's preserve them > > on the stack, along with the return address that might be above 4G as > > well. > > > > [0] Intel® 64 and IA-32 Architectures Software Developer’s Manual, Volume 1: Basic Architecture > > > > "Because the upper 32 bits of 64-bit general-purpose registers are > > undefined in 32-bit modes, the upper 32 bits of any general-purpose > > register are not preserved when switching from 64-bit mode to a 32-bit > > mode (to protected mode or compatibility mode). Software must not > > depend on these bits to maintain a value after a 64-bit to 32-bit > > mode switch." > > > > Fixes: 194a9749c73d650c ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/x86/boot/compressed/head_64.S | 23 +++++++++++++++----- > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > > index 03c4328a88cbd5d0..f7c11a0018477de8 100644 > > --- a/arch/x86/boot/compressed/head_64.S > > +++ b/arch/x86/boot/compressed/head_64.S > > @@ -459,11 +459,22 @@ SYM_CODE_START(startup_64) > > /* Save the trampoline address in RCX */ > > movq %rax, %rcx > > > > + /* Set up 32-bit addressable stack */ > > + leaq TRAMPOLINE_32BIT_STACK_END(%rcx), %rsp > > + > > /* > > - * Load the address of trampoline_return() into RDI. > > - * It will be used by the trampoline to return to the main code. > > + * Load the address of trampoline_return() into RDI and push it onto > > + * the stack so it will survive 32-bit truncation due to the 32-bit > > I think you should explain here what that SDM excerpt above says so that > it is clear what "32-bit truncation" is meant. > Ok > > + * protected mode switch. It will be used by the trampoline to return > > + * to the main code. > > */ > > leaq trampoline_return(%rip), %rdi > > + pushq %rdi > > + > > + /* Preserve other live 64-bit registers */ > > + pushq %rbp > > + pushq %rbx > > + pushq %rsi > > > > /* Switch to compatibility mode (CS.L = 0 CS.D = 1) via far return */ > > pushq $__KERNEL32_CS > > What is more interesting is why is this trampoline crap unconditional? > > /me goes and does git archeology... > > 194a9749c73d ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") > > We go through the trampoline even if we don't have to: if we're already > in 5-level paging mode or if we don't need to switch to it. This way the > trampoline gets tested on every boot. > > Well, f*ck no. > > All my machines don't have 5level pages. And I bet 5level pages is still > not in the majority of the machines out there. > > This all needs to be abstracted away into a separate function which > exits early if no 5 level support. > > Kirill, please fix this. > This is already fixed further down in the series.
On Mon, Jul 31, 2023 at 12:09:04PM +0200, Ard Biesheuvel wrote:
> This is already fixed further down in the series.
Thanks, lemme look.
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index 03c4328a88cbd5d0..f7c11a0018477de8 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -459,11 +459,22 @@ SYM_CODE_START(startup_64) /* Save the trampoline address in RCX */ movq %rax, %rcx + /* Set up 32-bit addressable stack */ + leaq TRAMPOLINE_32BIT_STACK_END(%rcx), %rsp + /* - * Load the address of trampoline_return() into RDI. - * It will be used by the trampoline to return to the main code. + * Load the address of trampoline_return() into RDI and push it onto + * the stack so it will survive 32-bit truncation due to the 32-bit + * protected mode switch. It will be used by the trampoline to return + * to the main code. */ leaq trampoline_return(%rip), %rdi + pushq %rdi + + /* Preserve other live 64-bit registers */ + pushq %rbp + pushq %rbx + pushq %rsi /* Switch to compatibility mode (CS.L = 0 CS.D = 1) via far return */ pushq $__KERNEL32_CS @@ -592,9 +603,6 @@ SYM_CODE_START(trampoline_32bit_src) movl %eax, %ds movl %eax, %ss - /* Set up new stack */ - leal TRAMPOLINE_32BIT_STACK_END(%ecx), %esp - /* Disable paging */ movl %cr0, %eax btrl $X86_CR0_PG_BIT, %eax @@ -671,7 +679,10 @@ SYM_CODE_END(trampoline_32bit_src) .code64 SYM_FUNC_START_LOCAL_NOALIGN(.Lpaging_enabled) /* Return from the trampoline */ - jmp *%rdi + popq %rsi + popq %rbx + popq %rbp + retq SYM_FUNC_END(.Lpaging_enabled) /*
The 4-to-5 level mode switch trampoline disables long mode and paging in order to be able to flick the LA57 bit. According to section 3.4.1.1 of the x86 architecture manual [0], we should not rely on 64-bit GPRs retaining the upper 32 bits of their contents across such a mode switch. Given that RBP, RBX and RSI are live at this point, let's preserve them on the stack, along with the return address that might be above 4G as well. [0] Intel® 64 and IA-32 Architectures Software Developer’s Manual, Volume 1: Basic Architecture "Because the upper 32 bits of 64-bit general-purpose registers are undefined in 32-bit modes, the upper 32 bits of any general-purpose register are not preserved when switching from 64-bit mode to a 32-bit mode (to protected mode or compatibility mode). Software must not depend on these bits to maintain a value after a 64-bit to 32-bit mode switch." Fixes: 194a9749c73d650c ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G") Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/x86/boot/compressed/head_64.S | 23 +++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-)