linux-kernel - Re: [PATCH 2/3] x86, cpu: Clean up and unify the NOP selection infrastructure

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: <1303169476.7181.71.camel@gandalf.stny.rr.com>
Date:	Mon, 18 Apr 2011 19:31:15 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	"H. Peter Anvin" <hpa@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org, Jason Baron <jbaron@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>, tglx@...utronix.de,
	mingo@...e.hu, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH 2/3] x86, cpu: Clean up and unify the NOP selection
 infrastructure

On Mon, 2011-04-18 at 15:35 -0700, H. Peter Anvin wrote:

>  #endif /* _ASM_X86_NOPS_H */
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 4a23467..2ca3f65 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -68,16 +68,20 @@ __setup("noreplace-paravirt", setup_noreplace_paravirt);
>  	printk(KERN_DEBUG fmt, args)
>  
>  #if defined(GENERIC_NOP1) && !defined(CONFIG_X86_64)
> -/* Use inline assembly to define this because the nops are defined
> -   as inline assembly strings in the include files and we cannot
> -   get them easily into strings. */
> -asm("\t" __stringify(__INITRODATA_OR_MODULE) "\nintelnops: "
> -	GENERIC_NOP1 GENERIC_NOP2 GENERIC_NOP3 GENERIC_NOP4 GENERIC_NOP5 GENERIC_NOP6
> -	GENERIC_NOP7 GENERIC_NOP8
> -    "\t.previous");
> -extern const unsigned char intelnops[];

Can we please add a comment to this. The original (above) was confusing
enough, but at least it used asm() so it wasn't that bad to figure out.
Or at least the asm() usage would trigger in one's mind to think "Damn!
They chose to use 'asm', it must be some kind of nasty trick. Let's take
a better look at WTF they are doing!".

Now the use a normal character array actual makes this even more subtle.
What about adding:

/*
 * Each GENERIC_NOPX is of X bytes, and defined as an array of bytes
 * that correspond to that nop. Getting from one nop to the next, we
 * add to the array the offset that is equal to the sum of all sizes of 
 * nops preceding the one we are after.
 *
 * Note: The GENERIC_NOP5_ATOMIC is at the end, as it breaks the
 * nice symmetry of sizes of the previous nops.
 */

-- Steve
 
> +static const unsigned char  __initconst_or_module intelnops[] =
> +{
> +	GENERIC_NOP1,
> +	GENERIC_NOP2,
> +	GENERIC_NOP3,
> +	GENERIC_NOP4,
> +	GENERIC_NOP5,
> +	GENERIC_NOP6,
> +	GENERIC_NOP7,
> +	GENERIC_NOP8,
> +	GENERIC_NOP5_ATOMIC
> +};
>  static const unsigned char *const __initconst_or_module
> -intel_nops[ASM_NOP_MAX+1] = {
> +intel_nops[ASM_NOP_MAX+2] = {
>  	NULL,
>  	intelnops,
>  	intelnops + 1,
> @@ -87,17 +91,25 @@ intel_nops[ASM_NOP_MAX+1] = {
>  	intelnops + 1 + 2 + 3 + 4 + 5,
>  	intelnops + 1 + 2 + 3 + 4 + 5 + 6,
>  	intelnops + 1 + 2 + 3 + 4 + 5 + 6 + 7,
> +	intelnops + 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8,
>  };
>  #endif


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