linux-kernel - Re: [PATCH v2 3/9] x86/paravirt: Replace paravirt patches with data

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: <alpine.DEB.2.21.1904192306120.3174@nanos.tec.linutronix.de>
Date:   Fri, 19 Apr 2019 23:26:50 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Andi Kleen <andi@...stfloor.org>
cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v2 3/9] x86/paravirt: Replace paravirt patches with
 data

On Fri, 29 Mar 2019, Andi Kleen wrote:
> For LTO all top level assembler statements need to be global because
> LTO might put it into a different assembler file than the referencing
> C code.
> 
> To avoid making all the paravirt patch snippets global replace them
> with data containing the patch instructions. Since these are unlikely
> to change this shouldn't be a significant maintenance burden.

s/with data containing/with unparseable, inconsistent and broken mess/

Unparseable:
------------

> +static const unsigned char patch_irq_save_fl[] = { 0x9c, 0x58 };  	/* pushf; pop %eax */
> +static const unsigned char patch_cpu_iret[] = { 0xcf };		  	/* iret */
> +static const unsigned char patch_mmu_read_cr2[] = { 0x0f, 0x20, 0xd0 }; /* mov %cr2, %eax */
> +static const unsigned char patch_mmu_write_cr3[] = { 0x0f, 0x22, 0xd8 };/* mov %eax, %cr3 */
> +static const unsigned char patch_mmu_read_cr3[] = { 0x0f, 0x20, 0xd8 }; /* mov %cr3, %eax */

  Overlong lines, spaces and tabs mixed, no formatting which allows easy
  reading and review.

Inconsistent and unparseable:
-----------------------------

>  #define PATCH_SITE(ops, x)					\
>  	case PARAVIRT_PATCH(ops.x):				\
> -		return paravirt_patch_insns(ibuf, len, start_##ops##_##x, end_##ops##_##x)
> +		return paravirt_patch_insns(ibuf, len, 		\
> +				patch_##ops##_##x, patch_##ops##_##x+sizeof(patch_##ops##_x));

vs.

> +		return paravirt_patch_insns(ibuf, len, start_##ops##_##x, \
> +				patch_##ops##_##x + sizeof(patch_##ops##_##x));

Broken:
-------

> +static const unsigned char patch_irq_restore_fl[] =  { 0x50, 0x9d};		/* pushq %rdi; popfq */

Can you spot the fail?

That probably works because Intel CPUs are so good at executing crappy
code, right? That's at least what you told me recently. If that's the proof
then I should really stop reviewing patches.

Thanks,

	tglx