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:	Sat, 4 Apr 2015 10:36:11 +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 Sat, Apr 04, 2015 at 09:34:54AM +0200, Borislav Petkov wrote:
> On Fri, Apr 03, 2015 at 10:42:17PM +0200, Quentin Casasnovas wrote:
> > If you're happy with the extra padding in such cases then your second
> > approach looks okay to me.  But IMO, even if taking the '.if' directive
> > approach is certainly bigger LOC-wise, it should be much easier to review
> > in a rush than some other .skip trickery.
> 
> .if needs absolute expressions and I can't get it to even compile with the
> experiments I've done so far.
> 
> How about this instead?
> 
> It basically computes the padding length by doing
> 
> max(len(repl1), len(repl2)) - len(orig)
>

Nice!

> and without conditionals. The macros all do string expansion so that the
> strings can get parsed by gas. Initial smoke testing in kvm seems to
> work, I need to test it on real metal:
>
> ---
> diff --git a/arch/x86/include/asm/alternative-asm.h b/arch/x86/include/asm/alternative-asm.h
> index 524bddce0b76..44a1fc5439d3 100644
> --- a/arch/x86/include/asm/alternative-asm.h
> +++ b/arch/x86/include/asm/alternative-asm.h
> @@ -45,12 +45,22 @@
>  	.popsection
>  .endm
>  
> +#define old_len			141b-140b
> +#define new_len1		144f-143f
> +#define new_len2		145f-144f
> +
> +/*
> + * Shamelessly stolen and adapted from:
> + * http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax
> + */
> +#define alt_max_short(a,b)	(((a) - (((a) - (b)) & (((a) - (b)) >> 15))) & 0xffff)
> +

Since all of these are compile time constants, could we not use the safe
variant on that same page?  Not that I'm too worried about the signed right
shift but heh that would be portable and should not impact performance
anyway, so no added value in using the optimized version is there?

>  .macro ALTERNATIVE_2 oldinstr, newinstr1, feature1, newinstr2, feature2
>  140:
>  	\oldinstr
>  141:
> -	.skip -(((144f-143f)-(141b-140b)) > 0) * ((144f-143f)-(141b-140b)),0x90
> -	.skip -(((145f-144f)-(144f-143f)-(141b-140b)) > 0) * ((145f-144f)-(144f-143f)-(141b-140b)),0x90
> +	.skip -((alt_max_short(new_len1, new_len2) - old_len) > 0) * \
> +		(alt_max_short(new_len1, new_len2) - old_len),0x90
>  142:
>  
>  	.pushsection .altinstructions,"a"
> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 5aef6a97d80e..2c515ebcc767 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -96,13 +96,19 @@ static inline int alternatives_text_reserved(void *start, void *end)
>  	alt_end_marker ":\n"
>  
>  /*
> + * max without conditionals. Shamelessly stolen and adapted from:
> + * http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax
> + */
> +#define alt_max_short(a, b)	"(((" a ") - (((" a ") - (" b ")) & (((" a ") - (" b ")) >> 15))) & 0xffff)"
> +
> +/*
>   * Pad the second replacement alternative with additional NOPs if it is
>   * additionally longer than the first replacement alternative.
>   */
> -#define OLDINSTR_2(oldinstr, num1, num2)					\
> -	__OLDINSTR(oldinstr, num1)						\
> -	".skip -(((" alt_rlen(num2) ")-(" alt_rlen(num1) ")-(662b-661b)) > 0) * " \
> -		"((" alt_rlen(num2) ")-(" alt_rlen(num1) ")-(662b-661b)),0x90\n"  \
> +#define OLDINSTR_2(oldinstr, num1, num2) \
> +	"661:\n\t" oldinstr "\n662:\n"								\
> +	".skip -((" alt_max_short(alt_rlen(num1), alt_rlen(num2)) " - (" alt_slen ")) > 0) * "	\
> +		"(" alt_max_short(alt_rlen(num1), alt_rlen(num2)) " - (" alt_slen ")), 0x90\n"	\
>  	alt_end_marker ":\n"

On the bonus side, you're getting rid of the double 'alt_end_marker' label
in case of an alternative_2()!

Looks good to me and I find it much easier to understand here :)

Thanks,
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/