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]

Date:	Thu, 2 Apr 2015 18:12:59 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Cc:	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 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

that's it. With exception handling of course.

> Are you not invariably clearing err here?  If the instruction fault, we go
> to label '3' which does 'err = -1; goto 2', which clears err.  Same remark
> for XSTATE_XSAVE()/XSTATE_RESTORE().
> 
> Probably missing something..

No, you're not. The backwards jump label needs to be after the XOR.
Thanks for catching that.

> Also, tiny consistency nit, maybe use "\n\t" everywhere?

Yeah, didn't want to make the macro more unreadable than it is now. The
"\n\t" things are only for when looking at the .s file and almost no one
does that :-)

> I've tried compiling this on top of v4.0-rc5 and I get a compile error
> because alt_end_marker isn't defined.  Which other patches should I take to
> test this?

It needs tip/master.

Let me redo the patch.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
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/