diff mbox

x86: Load __USER_DS into DS/ES after resume

Message ID 20150614065635.GA5294@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Ingo Molnar June 14, 2015, 6:56 a.m. UTC 
* Brian Gerst <brgerst@gmail.com> wrote:

> >> So if wakeup_pmode_return is really the first thing called then the whole 
> >> premise of shadow descriptor corruption goes out the window: we reload all 
> >> relevant segment registers.
> >
> > True, but it still leaves the fact that we're loading __KERNEL_DS instead of 
> > __USER_DS, right?  So we end up in the kernel in some context (I have no clue 
> > what context) with __KERNEL_DS loaded.  It's very easy for us to inadvertently 
> > fix it: we could return to userspace by any means whatsoever except SYSEXIT, 
> > or we could even return back to some preempted kernel context.
> >
> > I still think we should replace __KERNEL_DS with __USER_DS in 
> > wakeup_pmode_return and see if the problem goes away.
> 
> I'm pretty sure that's what the problem is.  If you look at the sysexit path, it 
> never reloads ds/es.  It assumes they are still __USER_DS set at sysenter.  The 
> iret path does restore all the user segments.

Ok, so something like the patch below, right?

Thanks,

	Ingo

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Pavel Machek June 14, 2015, 7:03 a.m. UTC | #1 
On Sun 2015-06-14 08:56:35, Ingo Molnar wrote:
> 
> * Brian Gerst <brgerst@gmail.com> wrote:
> 
> > >> So if wakeup_pmode_return is really the first thing called then the whole 
> > >> premise of shadow descriptor corruption goes out the window: we reload all 
> > >> relevant segment registers.
> > >
> > > True, but it still leaves the fact that we're loading __KERNEL_DS instead of 
> > > __USER_DS, right?  So we end up in the kernel in some context (I have no clue 
> > > what context) with __KERNEL_DS loaded.  It's very easy for us to inadvertently 
> > > fix it: we could return to userspace by any means whatsoever except SYSEXIT, 
> > > or we could even return back to some preempted kernel context.
> > >
> > > I still think we should replace __KERNEL_DS with __USER_DS in 
> > > wakeup_pmode_return and see if the problem goes away.
> > 
> > I'm pretty sure that's what the problem is.  If you look at the sysexit path, it 
> > never reloads ds/es.  It assumes they are still __USER_DS set at sysenter.  The 
> > iret path does restore all the user segments.
> 
> Ok, so something like the patch below, right?
> 
> Thanks,
> 
> 	Ingo
> 
> =====================>
>  arch/x86/kernel/acpi/wakeup_32.S | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
> index 665c6b7d2ea9..7302bbaea184 100644
> --- a/arch/x86/kernel/acpi/wakeup_32.S
> +++ b/arch/x86/kernel/acpi/wakeup_32.S
> @@ -12,11 +12,13 @@ ENTRY(wakeup_pmode_return)
>  wakeup_pmode_return:
>  	movw	$__KERNEL_DS, %ax
>  	movw	%ax, %ss
> -	movw	%ax, %ds
> -	movw	%ax, %es
>  	movw	%ax, %fs
>  	movw	%ax, %gs
>  
> +	movw	$__KERNEL_DS, %ax
> +	movw	%ax, %ds
> +	movw	%ax, %es

Umm. Are you sure? :-).
									Pavel
diff mbox

Patch

=====================>
 arch/x86/kernel/acpi/wakeup_32.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
index 665c6b7d2ea9..7302bbaea184 100644
--- a/arch/x86/kernel/acpi/wakeup_32.S
+++ b/arch/x86/kernel/acpi/wakeup_32.S
@@ -12,11 +12,13 @@  ENTRY(wakeup_pmode_return)
 wakeup_pmode_return:
 	movw	$__KERNEL_DS, %ax
 	movw	%ax, %ss
-	movw	%ax, %ds
-	movw	%ax, %es
 	movw	%ax, %fs
 	movw	%ax, %gs
 
+	movw	$__KERNEL_DS, %ax
+	movw	%ax, %ds
+	movw	%ax, %es
+
 	# reload the gdt, as we need the full 32 bit address
 	lidt	saved_idt
 	lldt	saved_ldt