linux-kernel - Re: [PATCH] x86/xsave: Robustify and merge macros

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]

Message-ID: <20150402163340.GB14902@chrystal.uk.oracle.com>
Date:	Thu, 2 Apr 2015 18:33:40 +0200
From:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Quentin Casasnovas <quentin.casasnovas@...cle.com>,
	X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Oleg Nesterov <oleg@...hat.com>,
	Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH] x86/xsave: Robustify and merge macros

On Thu, Apr 02, 2015 at 06:12:59PM +0200, Borislav Petkov wrote:
> On Thu, Apr 02, 2015 at 05:52:10PM +0200, Quentin Casasnovas wrote:
> > FWIW I think this looks much nicer!  I have a couple of comments though,
> > apologies in advance if they aren't relevant :)
> 
> No worries, I very much appreciate the looking at. :)
>

:)

> > I thought the SYSTEM_BOOTING checks were present to make sure we call these
> > functions only when the alternative instructions had *not* been applied
> > (i.e. when SYSTEM_BOOTING).  We could have added the opposite checks in
> > xsave_state()/xrstor_state() to make sure the alternative instructions are
> > applied when these are called (i.e. when !SYSTEM_BOOTING).
> 
> Well, I think this was a clumsy way to say that we shouldn't be using
> the _booting() variants when the system isn't booting anymore:
> 
> -       WARN_ON(system_state != SYSTEM_BOOTING);
>  
> -       if (boot_cpu_has(X86_FEATURE_XSAVES))
> -               asm volatile("1:"XSAVES"\n\t"
> -                       "2:\n\t"
> -                            xstate_fault
> -                       : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask)
> -                       :   "memory");
>         else
> -               asm volatile("1:"XSAVE"\n\t"
> -                       "2:\n\t"
> -                            xstate_fault
> -                       : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask)
> -                       :   "memory");
> 
> 
> WRT alternatives, the code didn't have any alternatives invocations
> there - it is just a cluttered way of saying:
> 
> 	if (CPU has XSAVES support)
> 		XSAVES
> 	else
> 		XSAVE

So I'm not saying the function is using alternative, just that if the
alternative _are_ applied, then we do not want to use the *_booting()
variants (likely for performance reasons), hence the WARN_ON().

So IMO it does not hurt to keep it here, with maybe renaming it something
like the following so it is obvious why it's there:

  /* Use the non _booting() variants if the alternatives are applied. */
  WARN_ON(altinstr_are_applied());

I would personnaly add it to the non _booting() variants as well to make
sure the alternative instructions _are_ applied, since otherwise that would
probably cause random failures to restore the xsaveopt/xsaves context
previously saved.  Obviously very paranoid check anyway so if you still
want to drop it then fine :)

Quentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/