linux-kernel - Re: [PATCH 12/27] x86/pkru: Provide .*_pkru

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:   Wed, 10 Apr 2019 18:36:15 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>
Subject: Re: [PATCH 12/27] x86/pkru: Provide .*_pkru_ins() functions

On Wed, Apr 03, 2019 at 06:41:41PM +0200, Sebastian Andrzej Siewior wrote:
> Dave Hansen has asked for __read_pkru() and __write_pkru() to be symmetrical.
> As part of the series __write_pkru() will read back the value and only write it
> if it is different.
> In order to make both functions symmetrical move the function containing only
> the opcode into a function with _isn() suffix. __write_pkru() will just invoke
> __write_pkru_isn() but in a flowup patch will also read back the value.
> 
> Suggested-by: Dave Hansen <dave.hansen@...el.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>  arch/x86/include/asm/pgtable.h       |  2 +-
>  arch/x86/include/asm/special_insns.h | 12 +++++++++---
>  arch/x86/kvm/vmx/vmx.c               |  2 +-
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 2779ace16d23f..64333e5222cd9 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -127,7 +127,7 @@ static inline int pte_dirty(pte_t pte)
>  static inline u32 read_pkru(void)
>  {
>  	if (boot_cpu_has(X86_FEATURE_OSPKE))
> -		return __read_pkru();
> +		return __read_pkru_ins();
>  	return 0;
>  }
>  
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 43c029cdc3fe8..27328606ff687 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -92,7 +92,7 @@ static inline void native_write_cr8(unsigned long val)
>  #endif
>  
>  #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> -static inline u32 __read_pkru(void)
> +static inline u32 __read_pkru_ins(void)
>  {
>  	u32 ecx = 0;
>  	u32 edx, pkru;
> @@ -107,7 +107,7 @@ static inline u32 __read_pkru(void)
>  	return pkru;
>  }
>  
> -static inline void __write_pkru(u32 pkru)
> +static inline void __write_pkru_ins(u32 pkru)
>  {
>  	u32 ecx = 0, edx = 0;
> 

Well, this is going in the wrong direction. The proper thing to do would
be to have:

rdpkru()
wrpkru()

which only do the inline asm with the respective opcodes. Just like
rdtsc(), clflush(), etc.

IOW, the functions which correspond to the instructions should be called
just like the respective instructions they implement in asm.

Then, all the helpers around them should have different names to denote
what they do. Like the current rdpkru() which does the assertion
checking should be renamed to something like read_pkey_user() or so.
Ditto for the current wrpkru().

Makes sense?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.