| Message ID | a155c20eec6343d34b3bdf40c024ce8b35a90e02.1662459668.git.baskov@ispras.ru |
|---|---|
| State | New |
| Headers | show |
| Series | x86_64: Improvements at compressed kernel stage | expand |
On Tue, 6 Sept 2022 at 12:41, Evgeniy Baskov <baskov@ispras.ru> wrote: > > Ensure WP bit to be set to prevent boot code from writing to > non-writable memory pages. > > Signed-off-by: Evgeniy Baskov <baskov@ispras.ru> > --- > 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 >
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
On 2022-10-19 10:06, Ard Biesheuvel wrote: > On Tue, 6 Sept 2022 at 12:41, Evgeniy Baskov <baskov@ispras.ru> wrote: >> >> Ensure WP bit to be set to prevent boot code from writing to >> non-writable memory pages. >> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru> >> --- >> 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 >>
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
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 */
Ensure WP bit to be set to prevent boot code from writing to non-writable memory pages. Signed-off-by: Evgeniy Baskov <baskov@ispras.ru> --- arch/x86/boot/compressed/head_64.S | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)