[PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

[PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

[Andy Lutomirski]

__restore_processor_context() had a couple of ordering bugs.  It
restored GSBASE after calling load_gs_index(), and the latter can
call into tracing code.  It also tried to restore segment registers
before restoring the LDT, which is straight-up wrong.

Reorder the code so that we restore GSBASE, then the descriptor
tables, then the segments.

This fixes two bugs.  First, it fixes a regression that broke resume
under certain configurations due to irqflag tracing in
native_load_gs_index().  Second, it fixes resume when the userspace
process that initiated suspect had funny segments.  The latter can be
reproduced by compiling this:

// SPDX-License-Identifier: GPL-2.0
/*
 * ldt_echo.c - Echo argv[1] while using an LDT segment
 */

int main(int argc, char **argv)
{
	int ret;
	size_t len;
	char *buf;

	const struct user_desc desc = {
                .entry_number    = 0,
                .base_addr       = 0,
                .limit           = 0xfffff,
                .seg_32bit       = 1,
                .contents        = 0, /* Data, grow-up */
                .read_exec_only  = 0,
                .limit_in_pages  = 1,
                .seg_not_present = 1,
                .useable         = 0
        };

	if (argc != 2)
		errx(1, "Usage: %s STRING", argv[0]);

	len = asprintf(&buf, "%s\n", argv[1]);
	if (len < 0)
		errx(1, "Out of memory");

	ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
	if (ret < -1)
		errno = -ret;
	if (ret)
		err(1, "modify_ldt");

	asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
	write(1, buf, len);
	return 0;
}

and running ldt_echo >/sys/power/mem

Without the fix, the latter causes a triple fault on resume.

Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Jarkko, can you test this version?

 arch/x86/power/cpu.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 84fcfde53f8f..5191de14f4df 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -226,8 +226,20 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_idt((const struct desc_ptr *)&ctxt->idt_limit);
 #endif
 
+#ifdef CONFIG_X86_64
 	/*
-	 * segment registers
+	 * We need GSBASE restored before percpu access can work.
+	 * percpu access can happen in exception handlers or in complicated
+	 * helpers like load_gs_index().
+	 */
+	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
+#endif
+
+	fix_processor_context();
+
+	/*
+	 * Restore segment registers.  This happens after restoring the GDT
+	 * and LDT, which happen in fix_processor_context().
 	 */
 #ifdef CONFIG_X86_32
 	loadsegment(es, ctxt->es);
@@ -248,13 +260,14 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_gs_index(ctxt->gs);
 	asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
 
+	/*
+	 * Restore FSBASE and user GSBASE after reloading the respective
+	 * segment selectors.
+	 */
 	wrmsrl(MSR_FS_BASE, ctxt->fs_base);
-	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
 	wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
 #endif
 
-	fix_processor_context();
-
 	do_fpu_end();
 	tsc_verify_tsc_adjust(true);
 	x86_platform.restore_sched_clock_state();
-- 
2.13.6

Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

[Andy Lutomirski]

On Thu, Nov 30, 2017 at 7:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
> __restore_processor_context() had a couple of ordering bugs.  It
> restored GSBASE after calling load_gs_index(), and the latter can
> call into tracing code.  It also tried to restore segment registers
> before restoring the LDT, which is straight-up wrong.

Ingo, Thomas, if you apply this version, can you fix up the following
changelog bug:

>                 .seg_not_present = 1,

That should be = 0 above.

If I send a v2, I'll fix it.

Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

[Thomas Gleixner]

On Thu, 30 Nov 2017, Andy Lutomirski wrote:

> On Thu, Nov 30, 2017 at 7:57 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > __restore_processor_context() had a couple of ordering bugs.  It
> > restored GSBASE after calling load_gs_index(), and the latter can
> > call into tracing code.  It also tried to restore segment registers
> > before restoring the LDT, which is straight-up wrong.
> 
> Ingo, Thomas, if you apply this version, can you fix up the following
> changelog bug:
> 
> >                 .seg_not_present = 1,
> 
> That should be = 0 above.

Sure. I'll wait for Jarkko to confirm.

> If I send a v2, I'll fix it.
> 

Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor_context()

[Jarkko Nikula]

On 11/30/2017 05:57 PM, Andy Lutomirski wrote:
> __restore_processor_context() had a couple of ordering bugs.  It
> restored GSBASE after calling load_gs_index(), and the latter can
> call into tracing code.  It also tried to restore segment registers
> before restoring the LDT, which is straight-up wrong.
> 
> Reorder the code so that we restore GSBASE, then the descriptor
> tables, then the segments.
> 
> This fixes two bugs.  First, it fixes a regression that broke resume
> under certain configurations due to irqflag tracing in
> native_load_gs_index().  Second, it fixes resume when the userspace
> process that initiated suspect had funny segments.  The latter can be
> reproduced by compiling this:
> 
> // SPDX-License-Identifier: GPL-2.0
> /*
>   * ldt_echo.c - Echo argv[1] while using an LDT segment
>   */
> 
> int main(int argc, char **argv)
> {
> 	int ret;
> 	size_t len;
> 	char *buf;
> 
> 	const struct user_desc desc = {
>                  .entry_number    = 0,
>                  .base_addr       = 0,
>                  .limit           = 0xfffff,
>                  .seg_32bit       = 1,
>                  .contents        = 0, /* Data, grow-up */
>                  .read_exec_only  = 0,
>                  .limit_in_pages  = 1,
>                  .seg_not_present = 1,
>                  .useable         = 0
>          };
> 
> 	if (argc != 2)
> 		errx(1, "Usage: %s STRING", argv[0]);
> 
> 	len = asprintf(&buf, "%s\n", argv[1]);
> 	if (len < 0)
> 		errx(1, "Out of memory");
> 
> 	ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
> 	if (ret < -1)
> 		errno = -ret;
> 	if (ret)
> 		err(1, "modify_ldt");
> 
> 	asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
> 	write(1, buf, len);
> 	return 0;
> }
> 
> and running ldt_echo >/sys/power/mem
> 
> Without the fix, the latter causes a triple fault on resume.
> 
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> 
> Jarkko, can you test this version?
> 
>   arch/x86/power/cpu.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
It does fix the suspend/resume issue I saw. Patch applied on top of 
current head df8ba95c572a and a loop below completed fine on a few 
machines where I tested it. All of them had issue with the ca37e57bbe0c.

for ((i=0;i<10;i++)); do rtcwake -s 5 -m mem; echo $i; done

Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

[tip:x86/urgent] x86/power: Fix some ordering bugs in __restore_processor_context()

[tip-bot for Andy Lutomirski]

Commit-ID:  cdf577209aad4cdbe3455d3efa6cf631f838c55d
Gitweb:     https://git.kernel.org/tip/cdf577209aad4cdbe3455d3efa6cf631f838c55d
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Nov 2017 07:57:57 -0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 4 Dec 2017 23:41:42 +0100

x86/power: Fix some ordering bugs in __restore_processor_context()

__restore_processor_context() had a couple of ordering bugs.  It
restored GSBASE after calling load_gs_index(), and the latter can
call into tracing code.  It also tried to restore segment registers
before restoring the LDT, which is straight-up wrong.

Reorder the code so that we restore GSBASE, then the descriptor
tables, then the segments.

This fixes two bugs.  First, it fixes a regression that broke resume
under certain configurations due to irqflag tracing in
native_load_gs_index().  Second, it fixes resume when the userspace
process that initiated suspect had funny segments.  The latter can be
reproduced by compiling this:

// SPDX-License-Identifier: GPL-2.0
/*
 * ldt_echo.c - Echo argv[1] while using an LDT segment
 */

int main(int argc, char **argv)
{
	int ret;
	size_t len;
	char *buf;

	const struct user_desc desc = {
                .entry_number    = 0,
                .base_addr       = 0,
                .limit           = 0xfffff,
                .seg_32bit       = 1,
                .contents        = 0, /* Data, grow-up */
                .read_exec_only  = 0,
                .limit_in_pages  = 1,
                .seg_not_present = 0,
                .useable         = 0
        };

	if (argc != 2)
		errx(1, "Usage: %s STRING", argv[0]);

	len = asprintf(&buf, "%s\n", argv[1]);
	if (len < 0)
		errx(1, "Out of memory");

	ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
	if (ret < -1)
		errno = -ret;
	if (ret)
		err(1, "modify_ldt");

	asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
	write(1, buf, len);
	return 0;
}

and running ldt_echo >/sys/power/mem

Without the fix, the latter causes a triple fault on resume.

Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lkml.kernel.org/r/6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org

---
 arch/x86/power/cpu.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 84fcfde..5191de1 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -226,8 +226,20 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_idt((const struct desc_ptr *)&ctxt->idt_limit);
 #endif
 
+#ifdef CONFIG_X86_64
 	/*
-	 * segment registers
+	 * We need GSBASE restored before percpu access can work.
+	 * percpu access can happen in exception handlers or in complicated
+	 * helpers like load_gs_index().
+	 */
+	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
+#endif
+
+	fix_processor_context();
+
+	/*
+	 * Restore segment registers.  This happens after restoring the GDT
+	 * and LDT, which happen in fix_processor_context().
 	 */
 #ifdef CONFIG_X86_32
 	loadsegment(es, ctxt->es);
@@ -248,13 +260,14 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_gs_index(ctxt->gs);
 	asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
 
+	/*
+	 * Restore FSBASE and user GSBASE after reloading the respective
+	 * segment selectors.
+	 */
 	wrmsrl(MSR_FS_BASE, ctxt->fs_base);
-	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
 	wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
 #endif
 
-	fix_processor_context();
-
 	do_fpu_end();
 	tsc_verify_tsc_adjust(true);
 	x86_platform.restore_sched_clock_state();

[tip:x86/urgent] x86/power: Fix some ordering bugs in __restore_processor_context()

[tip-bot for Andy Lutomirski]

Commit-ID:  5b06bbcfc2c621da3009da8decb7511500c293ed
Gitweb:     https://git.kernel.org/tip/5b06bbcfc2c621da3009da8decb7511500c293ed
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Nov 2017 07:57:57 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Dec 2017 12:29:12 +0100

x86/power: Fix some ordering bugs in __restore_processor_context()

__restore_processor_context() had a couple of ordering bugs.  It
restored GSBASE after calling load_gs_index(), and the latter can
call into tracing code.  It also tried to restore segment registers
before restoring the LDT, which is straight-up wrong.

Reorder the code so that we restore GSBASE, then the descriptor
tables, then the segments.

This fixes two bugs.  First, it fixes a regression that broke resume
under certain configurations due to irqflag tracing in
native_load_gs_index().  Second, it fixes resume when the userspace
process that initiated suspect had funny segments.  The latter can be
reproduced by compiling this:

// SPDX-License-Identifier: GPL-2.0
/*
 * ldt_echo.c - Echo argv[1] while using an LDT segment
 */

int main(int argc, char **argv)
{
	int ret;
	size_t len;
	char *buf;

	const struct user_desc desc = {
                .entry_number    = 0,
                .base_addr       = 0,
                .limit           = 0xfffff,
                .seg_32bit       = 1,
                .contents        = 0, /* Data, grow-up */
                .read_exec_only  = 0,
                .limit_in_pages  = 1,
                .seg_not_present = 0,
                .useable         = 0
        };

	if (argc != 2)
		errx(1, "Usage: %s STRING", argv[0]);

	len = asprintf(&buf, "%s\n", argv[1]);
	if (len < 0)
		errx(1, "Out of memory");

	ret = syscall(SYS_modify_ldt, 1, &desc, sizeof(desc));
	if (ret < -1)
		errno = -ret;
	if (ret)
		err(1, "modify_ldt");

	asm volatile ("movw %0, %%es" :: "rm" ((unsigned short)7));
	write(1, buf, len);
	return 0;
}

and running ldt_echo >/sys/power/mem

Without the fix, the latter causes a triple fault on resume.

Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lkml.kernel.org/r/6b31721ea92f51ea839e79bd97ade4a75b1eeea2.1512057304.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/power/cpu.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 84fcfde..5191de1 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -226,8 +226,20 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_idt((const struct desc_ptr *)&ctxt->idt_limit);
 #endif
 
+#ifdef CONFIG_X86_64
 	/*
-	 * segment registers
+	 * We need GSBASE restored before percpu access can work.
+	 * percpu access can happen in exception handlers or in complicated
+	 * helpers like load_gs_index().
+	 */
+	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
+#endif
+
+	fix_processor_context();
+
+	/*
+	 * Restore segment registers.  This happens after restoring the GDT
+	 * and LDT, which happen in fix_processor_context().
 	 */
 #ifdef CONFIG_X86_32
 	loadsegment(es, ctxt->es);
@@ -248,13 +260,14 @@ static void notrace __restore_processor_state(struct saved_context *ctxt)
 	load_gs_index(ctxt->gs);
 	asm volatile ("movw %0, %%ss" :: "r" (ctxt->ss));
 
+	/*
+	 * Restore FSBASE and user GSBASE after reloading the respective
+	 * segment selectors.
+	 */
 	wrmsrl(MSR_FS_BASE, ctxt->fs_base);
-	wrmsrl(MSR_GS_BASE, ctxt->gs_base);
 	wrmsrl(MSR_KERNEL_GS_BASE, ctxt->gs_kernel_base);
 #endif
 
-	fix_processor_context();
-
 	do_fpu_end();
 	tsc_verify_tsc_adjust(true);
 	x86_platform.restore_sched_clock_state();