2022-09-06 10:43:22

by Evgeniy Baskov

Subject: [PATCH 03/16] x86/boot: Set cr0 to known state in trampoline

Ensure WP bit to be set to prevent boot code from writing to
non-writable memory pages.

Signed-off-by: Evgeniy Baskov <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index d33f060900d2..5273367283b7 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -619,9 +619,8 @@ SYM_CODE_START(trampoline_32bit_src)
/* Set up new stack */
leal TRAMPOLINE_32BIT_STACK_END(%ecx), %esp

- /* Disable paging */
- movl %cr0, %eax
- btrl $X86_CR0_PG_BIT, %eax
+ /* Disable paging and setup CR0 */
+ movl $(CR0_STATE & ~X86_CR0_PG), %eax
movl %eax, %cr0

/* Check what paging mode we want to be in after the trampoline */
--
2.35.1

2022-10-19 07:36:28

by Ard Biesheuvel

Subject: Re: [PATCH 03/16] x86/boot: Set cr0 to known state in trampoline

On Tue, 6 Sept 2022 at 12:41, Evgeniy Baskov <[email protected]> wrote:
>
> Ensure WP bit to be set to prevent boot code from writing to
> non-writable memory pages.
>
> Signed-off-by: Evgeniy Baskov <[email protected]>
> ---
> arch/x86/boot/compressed/head_64.S | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index d33f060900d2..5273367283b7 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -619,9 +619,8 @@ SYM_CODE_START(trampoline_32bit_src)
> /* Set up new stack */
> leal TRAMPOLINE_32BIT_STACK_END(%ecx), %esp
>
> - /* Disable paging */
> - movl %cr0, %eax
> - btrl $X86_CR0_PG_BIT, %eax

Why do we no longer care about CR0's prior value?

> + /* Disable paging and setup CR0 */
> + movl $(CR0_STATE & ~X86_CR0_PG), %eax
> movl %eax, %cr0
>
> /* Check what paging mode we want to be in after the trampoline */
> --
> 2.35.1
>

2022-10-20 11:59:52

by Evgeniy Baskov

Subject: Re: [PATCH 03/16] x86/boot: Set cr0 to known state in trampoline

On 2022-10-19 10:06, Ard Biesheuvel wrote:
> On Tue, 6 Sept 2022 at 12:41, Evgeniy Baskov <[email protected]> wrote:
>>
>> Ensure WP bit to be set to prevent boot code from writing to
>> non-writable memory pages.
>>
>> Signed-off-by: Evgeniy Baskov <[email protected]>
>> ---
>> arch/x86/boot/compressed/head_64.S | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/head_64.S
>> b/arch/x86/boot/compressed/head_64.S
>> index d33f060900d2..5273367283b7 100644
>> --- a/arch/x86/boot/compressed/head_64.S
>> +++ b/arch/x86/boot/compressed/head_64.S
>> @@ -619,9 +619,8 @@ SYM_CODE_START(trampoline_32bit_src)
>> /* Set up new stack */
>> leal TRAMPOLINE_32BIT_STACK_END(%ecx), %esp
>>
>> - /* Disable paging */
>> - movl %cr0, %eax
>> - btrl $X86_CR0_PG_BIT, %eax
>
> Why do we no longer care about CR0's prior value?

I think we don't need to preserve any of those flags
(we nether use floating point instructions nor call EFI functions
with this cr0 value) and it's better to set cr0 to the well-known
state. CR0 is also being set to the constant value while switching
from protected to long mode, so it is already done in one of the
code paths.

If I am missing something, let me know,
I will change it to only set WP and clear PG.

>
>> + /* Disable paging and setup CR0 */
>> + movl $(CR0_STATE & ~X86_CR0_PG), %eax
>> movl %eax, %cr0
>>
>> /* Check what paging mode we want to be in after the
>> trampoline */
>> --
>> 2.35.1
>>

2022-10-20 13:40:37

by Evgeniy Baskov

Subject: Re: [PATCH 03/16] x86/boot: Set cr0 to known state in trampoline

On 2022-10-19 10:44, Andrew Cooper wrote:
> On 06/09/2022 11:41, Evgeniy Baskov wrote:
>> diff --git a/arch/x86/boot/compressed/head_64.S
>> b/arch/x86/boot/compressed/head_64.S
>> index d33f060900d2..5273367283b7 100644
>> --- a/arch/x86/boot/compressed/head_64.S
>> +++ b/arch/x86/boot/compressed/head_64.S
>> @@ -619,9 +619,8 @@ SYM_CODE_START(trampoline_32bit_src)
>> /* Set up new stack */
>> leal TRAMPOLINE_32BIT_STACK_END(%ecx), %esp
>>
>> - /* Disable paging */
>> - movl %cr0, %eax
>> - btrl $X86_CR0_PG_BIT, %eax
>> + /* Disable paging and setup CR0 */
>> + movl $(CR0_STATE & ~X86_CR0_PG), %eax
>
> Why here?  WP is ignored when PG is disabled.
>
> ~Andrew

PG is enabled lower in this function, so WP can also be set there,
it should not make any difference. The only important thing is that
WP supposed to be set in trampoline code.

If you think, that it would be more logical to set PG and WP
simultaneously, I can change it to be that way.

Thanks,
Evgeniy Baskov