x86/entry/32: Fix setup of CS high bits
diff mbox series

Message ID 35a24feb-5970-aa03-acbf-53428a159ace@web.de
State New
Headers show
Series
  • x86/entry/32: Fix setup of CS high bits
Related show

Commit Message

Jan Kiszka Oct. 13, 2018, 9:54 a.m. UTC
From: Jan Kiszka <jan.kiszka@siemens.com>

Even if we are not on an entry stack, we have to initialize the CS high
bits because we are unconditionally evaluating them
PARANOID_EXIT_TO_KERNEL_MODE. Failing to do so broke the boot on Galileo
Gen2 and IOT2000 boards.

Fixes: b92a165df17e ("x86/entry/32: Handle Entry from Kernel-Mode on Entry-Stack")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/entry/entry_32.S | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Andy Lutomirski Oct. 13, 2018, 3:12 p.m. UTC | #1
On Sat, Oct 13, 2018 at 3:02 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Even if we are not on an entry stack, we have to initialize the CS high
> bits because we are unconditionally evaluating them
> PARANOID_EXIT_TO_KERNEL_MODE. Failing to do so broke the boot on Galileo
> Gen2 and IOT2000 boards.
>
> Fixes: b92a165df17e ("x86/entry/32: Handle Entry from Kernel-Mode on Entry-Stack")
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/entry/entry_32.S | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 2767c625a52c..95c94d48ecd2 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -389,6 +389,12 @@
>          * that register for the time this macro runs
>          */
>
> +       /*
> +        * Clear unused upper bits of the dword containing the word-sized CS
> +        * slot in pt_regs in case hardware didn't clear it for us.
> +        */
> +       andl    $(0x0000ffff), PT_CS(%esp)
> +

Please improve the comment. Since commit:

commit 385eca8f277c4c34f361a4c3a088fd876d29ae21
Author: Andy Lutomirski <luto@kernel.org>
Date:   Fri Jul 28 06:00:30 2017 -0700

    x86/asm/32: Make pt_regs's segment registers be 16 bits

Those fields are genuinely 16 bit.  So the comment should say
something like "Those high bits are used for CS_FROM_ENTRY_STACK and
CS_FROM_USER_CR3".

Also, can you fold something like this in:

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 2767c625a52c..358eed8cf62a 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -171,7 +171,7 @@
        ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
        .if \no_user_check == 0
        /* coming from usermode? */
-       testl   $SEGMENT_RPL_MASK, PT_CS(%esp)
+       testb   $SEGMENT_RPL_MASK, PT_CS(%esp)
        jz      .Lend_\@
        .endif
        /* On user-cr3? */
Joerg Roedel Oct. 15, 2018, 9:10 a.m. UTC | #2
Hey Jan,

thanks for tracking this down and sending the fix! So your hardware
probably doesn't zero out the CS high bits, so that the code wrongly
detects that it came from the entry stack on return. Clearing the bits
earlier before the entry-stack check makes sense.

Acked-by: Joerg Roedel <jroedel@suse.de>
Reviewed-by: Joerg Roedel <jroedel@suse.de>

On Sat, Oct 13, 2018 at 11:54:54AM +0200, Jan Kiszka wrote:
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 2767c625a52c..95c94d48ecd2 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -389,6 +389,12 @@
>  	 * that register for the time this macro runs
>  	 */
>  
> +	/*
> +	 * Clear unused upper bits of the dword containing the word-sized CS
> +	 * slot in pt_regs in case hardware didn't clear it for us.
> +	 */
> +	andl	$(0x0000ffff), PT_CS(%esp)
> +
>  	/* Are we on the entry stack? Bail out if not! */
>  	movl	PER_CPU_VAR(cpu_entry_area), %ecx
>  	addl	$CPU_ENTRY_AREA_entry_stack + SIZEOF_entry_stack, %ecx
> @@ -407,12 +413,6 @@
>  	/* Load top of task-stack into %edi */
>  	movl	TSS_entry2task_stack(%edi), %edi
>  
> -	/*
> -	 * Clear unused upper bits of the dword containing the word-sized CS
> -	 * slot in pt_regs in case hardware didn't clear it for us.
> -	 */
> -	andl	$(0x0000ffff), PT_CS(%esp)
> -
>  	/* Special case - entry from kernel mode via entry stack */
>  #ifdef CONFIG_VM86
>  	movl	PT_EFLAGS(%esp), %ecx		# mix EFLAGS and CS
> -- 
> 2.16.4
Jan Kiszka Oct. 15, 2018, 1:08 p.m. UTC | #3
On 13.10.18 17:12, Andy Lutomirski wrote:
> On Sat, Oct 13, 2018 at 3:02 AM Jan Kiszka <jan.kiszka@web.de> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Even if we are not on an entry stack, we have to initialize the CS high
>> bits because we are unconditionally evaluating them
>> PARANOID_EXIT_TO_KERNEL_MODE. Failing to do so broke the boot on Galileo
>> Gen2 and IOT2000 boards.
>>
>> Fixes: b92a165df17e ("x86/entry/32: Handle Entry from Kernel-Mode on Entry-Stack")
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>   arch/x86/entry/entry_32.S | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
>> index 2767c625a52c..95c94d48ecd2 100644
>> --- a/arch/x86/entry/entry_32.S
>> +++ b/arch/x86/entry/entry_32.S
>> @@ -389,6 +389,12 @@
>>           * that register for the time this macro runs
>>           */
>>
>> +       /*
>> +        * Clear unused upper bits of the dword containing the word-sized CS
>> +        * slot in pt_regs in case hardware didn't clear it for us.
>> +        */
>> +       andl    $(0x0000ffff), PT_CS(%esp)
>> +
> 
> Please improve the comment. Since commit:
> 
> commit 385eca8f277c4c34f361a4c3a088fd876d29ae21
> Author: Andy Lutomirski <luto@kernel.org>
> Date:   Fri Jul 28 06:00:30 2017 -0700
> 
>      x86/asm/32: Make pt_regs's segment registers be 16 bits
> 
> Those fields are genuinely 16 bit.  So the comment should say
> something like "Those high bits are used for CS_FROM_ENTRY_STACK and
> CS_FROM_USER_CR3".

/*
  * The high bits of the CS dword (__csh) are used for
  * CS_FROM_ENTRY_STACK and CS_FROM_USER_CR3. Clear them in case
  * hardware didn't do this for us.
  */

OK? I will send out v2 with this wording soon.

> 
> Also, can you fold something like this in:
> 
> diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> index 2767c625a52c..358eed8cf62a 100644
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -171,7 +171,7 @@
>          ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
>          .if \no_user_check == 0
>          /* coming from usermode? */
> -       testl   $SEGMENT_RPL_MASK, PT_CS(%esp)
> +       testb   $SEGMENT_RPL_MASK, PT_CS(%esp)
>          jz      .Lend_\@
>          .endif
>          /* On user-cr3? */
> 

Makes sense, but this looks like a separate patch to me.

Jan
David Laight Oct. 15, 2018, 1:14 p.m. UTC | #4
From: Jan Kiszka
> Sent: 15 October 2018 14:09
...
> > Those fields are genuinely 16 bit.  So the comment should say
> > something like "Those high bits are used for CS_FROM_ENTRY_STACK and
> > CS_FROM_USER_CR3".
> 
> /*
>   * The high bits of the CS dword (__csh) are used for
>   * CS_FROM_ENTRY_STACK and CS_FROM_USER_CR3. Clear them in case
>   * hardware didn't do this for us.
>   */

What's a 'dword' ? :-)

On a 32bit processor a 'word' will be 32 bits to a 'double-word'
would be 64 bits.
One of the worst names to use.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Jan Kiszka Oct. 15, 2018, 1:18 p.m. UTC | #5
On 15.10.18 15:14, David Laight wrote:
> From: Jan Kiszka
>> Sent: 15 October 2018 14:09
> ...
>>> Those fields are genuinely 16 bit.  So the comment should say
>>> something like "Those high bits are used for CS_FROM_ENTRY_STACK and
>>> CS_FROM_USER_CR3".
>>
>> /*
>>    * The high bits of the CS dword (__csh) are used for
>>    * CS_FROM_ENTRY_STACK and CS_FROM_USER_CR3. Clear them in case
>>    * hardware didn't do this for us.
>>    */
> 
> What's a 'dword' ? :-)
> 
> On a 32bit processor a 'word' will be 32 bits to a 'double-word'
> would be 64 bits.
> One of the worst names to use.

That's ia32 nomenclature: a doubleword (dword) is a 32-bit value.

Jan
David Laight Oct. 15, 2018, 1:29 p.m. UTC | #6
From: Jan Kiszka
> On 15.10.18 15:14, David Laight wrote:
> > From: Jan Kiszka
> >> Sent: 15 October 2018 14:09
> > ...
> >>> Those fields are genuinely 16 bit.  So the comment should say
> >>> something like "Those high bits are used for CS_FROM_ENTRY_STACK and
> >>> CS_FROM_USER_CR3".
> >>
> >> /*
> >>    * The high bits of the CS dword (__csh) are used for
> >>    * CS_FROM_ENTRY_STACK and CS_FROM_USER_CR3. Clear them in case
> >>    * hardware didn't do this for us.
> >>    */
> >
> > What's a 'dword' ? :-)
> >
> > On a 32bit processor a 'word' will be 32 bits to a 'double-word'
> > would be 64 bits.
> > One of the worst names to use.
> 
> That's ia32 nomenclature: a doubleword (dword) is a 32-bit value.

I think you missed the :-)
I don't think linux uses that term very often.

Any guesses as to what type DWORD_PTR is?
(in a well know 64bit environment that uses UPPER_CASE for types).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch
diff mbox series

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 2767c625a52c..95c94d48ecd2 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -389,6 +389,12 @@ 
 	 * that register for the time this macro runs
 	 */
 
+	/*
+	 * Clear unused upper bits of the dword containing the word-sized CS
+	 * slot in pt_regs in case hardware didn't clear it for us.
+	 */
+	andl	$(0x0000ffff), PT_CS(%esp)
+
 	/* Are we on the entry stack? Bail out if not! */
 	movl	PER_CPU_VAR(cpu_entry_area), %ecx
 	addl	$CPU_ENTRY_AREA_entry_stack + SIZEOF_entry_stack, %ecx
@@ -407,12 +413,6 @@ 
 	/* Load top of task-stack into %edi */
 	movl	TSS_entry2task_stack(%edi), %edi
 
-	/*
-	 * Clear unused upper bits of the dword containing the word-sized CS
-	 * slot in pt_regs in case hardware didn't clear it for us.
-	 */
-	andl	$(0x0000ffff), PT_CS(%esp)
-
 	/* Special case - entry from kernel mode via entry stack */
 #ifdef CONFIG_VM86
 	movl	PT_EFLAGS(%esp), %ecx		# mix EFLAGS and CS