linux-kernel - Re: [PATCH] x86/power: Fix some ordering bugs in __restore_processor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2017 11:06:11 +0200
From:   Jarkko Nikula <jarkko.nikula@...ux.intel.com>
To:     Andy Lutomirski <luto@...nel.org>, x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, Borislav Petkov <bp@...en8.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] x86/power: Fix some ordering bugs in
 __restore_processor_context()

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@...ux.intel.com>
> Fixes: ca37e57bbe0c ("x86/entry/64: Add missing irqflags tracing to native_load_gs_index()")
> Signed-off-by: Andy Lutomirski <luto@...nel.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@...ux.intel.com>