| Message ID | 35a24feb-5970-aa03-acbf-53428a159ace@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/entry/32: Fix setup of CS high bits | expand |
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? */
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
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
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)
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
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)
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