linux-kernel - Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore

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: <20150318095356.GC3411@pd.tnic>
Date:	Wed, 18 Mar 2015 10:53:56 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	Dave Hansen <dave.hansen@...el.com>,
	Ingo Molnar <mingo@...nel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Pekka Riikonen <priikone@....fi>,
	Rik van Riel <riel@...hat.com>,
	Suresh Siddha <sbsiddha@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"Yu, Fenghua" <fenghua.yu@...el.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in
 xsave_user/xrestore_user

On Wed, Mar 18, 2015 at 10:06:32AM +0100, Quentin Casasnovas wrote:
> What if we renamed it to check_asm()/check_user_asm() and have the first
> argument be a string, like an asm statement?  So basically check_asm()
> would be exactly like an asm() statement except that it'll use a comma to
> separate the input, output and clobber operands instead of a colon, and
> would protect the first instruction of the assembler template.
> 
>         if (config_enabled(CONFIG_X86_32))
>                 return check_user_asm("fxrstor %[fx]", [fx] "=m" (*fx),,);
> 
> Then we can move that macro up the headers so it can be used elsewhere.

Actually, I don't like the variable arguments thing and am not sure at
all that there's a wide need for a check* thing across the tree. Maybe
there is but I haven't seen it yet.

So I'd much prefer macros of the sort:

	fxsave()
	xsave()
	xsaves()
	xrstor()
	...

(no need for the "check" thing)

which are self contained and get passed the needed operands. I.e.,

	fxsave(fx)

and fx is "struct i387_fxsave_struct __user *fx". We can wrap it in
inline functions for arguments checking too.

Also:

	xsave(state, lmask, hmask)

and the macro definition does the exception table thing. And we can have
a lower level __save_state() macro which is getting called by all those
so that we can save us the code duplication.

This is much cleaner IMO than the check_insn() things.

> Readability will be a tough one since gcc extended asm isn't readable
> (IMO) and we need to deal with the input/output/clobber operands
> syntax.

That's why I'm saying we wrap all that inline asm syntax in macros and
not pass inline asm-like but not really arguments to our macros.

> I do agree with all your above points, which is why I drafted that
> proposal rework of check_insn() in my first e-mail :) AFAICT, you were
> giving arguments against the current macros, not against my previous
> proposal.

All I'm saying is, it should be done cleanly instead of improving an
already not so optimal design.

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/