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]

Date:	Mon, 16 Mar 2015 23:37:44 +0100
From:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Dave Hansen <dave.hansen@...el.com>, Borislav Petkov <bp@...e.de>,
	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>,
	Quentin Casasnovas <quentin.casasnovas@...cle.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in
 xsave_user/xrestore_user

On Sun, Mar 15, 2015 at 05:49:48PM +0100, Oleg Nesterov wrote:
> Hello.
> 
> Another a bit off-topic change, but I'd like to finish the discussion
> with Quentin.
>
> And almost cosmetic. But I added the RFC tag to make it clear that this
> needs a review from someone who understands gcc-asm better. In particular
> I am worried if that dummy "=m" (*buf) is actually correct.

Derp, I might have given the wrong impression but I'm certainly not that
guy who understands gcc-asm better!

> 
> And I agree with Quentin, user_insn/check_insn can be improved to allow
> clobbers, more flexible "output", etc. But imo they already can make this
> code look a bit better, and "xstate_fault" must die eventually.
>

So I really think we should have a clean user_insn() for which we could add
arbitrary outputs, inputs and clobbers and which would follow more closely
the extended assembly syntax (so it's easier to parse for the elite who
actually understands GCC extended asm ;).  Would something like this be
Okay?

/**
 * This is so multiple outputs/inputs/clobbers are interpreted as a
 * single macro argument.
 */
#define SINGLE_ARG(...) __VA_ARGS__

#define PASTE_1(...) , __VA_ARGS__
#define PASTE_0(...)
#define PASTE__(Empty, ...) PASTE_ ## Empty(__VA_ARGS__)
#define PASTE_(Empty, ...) PASTE__(Empty, __VA_ARGS__)

#define IS_EMPTY(...) 1

/**
 * Prints ", __VA_ARGS__" (note the comma as prefix) if there are any
 * argument and does not print anything otherwise.
 */
#define PASTE(...) PASTE_(IS_EMPTY(__VA_ARGS), __VA_ARGS__)

#define __user_insn(stingified_instructions, outputs, inputs, clobbers) \
	({								\
		int err;						\
		asm volatile(ASM_STAC                  "\n\t"		\
	                     "try:                      \n\t"		\
	                     stringified_instructions			\
	                     "finally:                  \n\t"		\
	                     ASM_CLAC                  "\n\t"		\
			     ".section .fixup,\"ax\"    \n\t"		\
			     "catch:  movl $-1,%[err]   \n\t"		\
			     "    jmp  finallyb         \n\t"		\
			     ".previous                 \n\t"		\
			     _ASM_EXTABLE(tryb, catchb)			\
			     : [err] "=r" (err) PASTE(outputs)		\
			     : "0" (0) PASTE(inputs)			\
			     : clobbers)				\
			err;						\
	})

Then the callers can use SINGLE_ARG() to pass multiple outputs, inputs or
clobbers as a single argument to __user_insn like this:

  __user_insn("btl [var2], %0		\n\t",
  	      , /* no outputs, no need for dummy arg */
	      SINGLE_ARG("r" (var1), [var2] "r" (var2)), /* two inputs */
	      "cc");

As added bonus, we would not need any dummy operand when there is no
outputs, which I think is preferable.

The IS_EMPTY() macro could be implemented like on the following article
  https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/

We could then write safe variants of the alternative_*() macros to call
check_insn() since I don't think it is possible as is to do
check_insn(alternative_2(...), ...) as you suggested.

>
> Quentin, could you review? I can't find your last email about this change,
> and I can't recall if you agree or not.
> 

I can give it a try, but really not the best person to ack/nack this kind
of changes..

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/