linux-kernel - Re: [RFC Part1 PATCH v3 03/17] x86/mm: Secure Encrypted Virtualization (SEV) support

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <facf3ac6-ebda-57a7-f961-6029b3ac7be7@amd.com>
Date:   Wed, 26 Jul 2017 11:47:32 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Borislav Petkov <bp@...e.de>, Brijesh Singh <brijesh.singh@....com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        linux-efi@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Piotr Luc <piotr.luc@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Reza Arbab <arbab@...ux.vnet.ibm.com>,
        David Howells <dhowells@...hat.com>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Laura Abbott <labbott@...hat.com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Eric Biederman <ebiederm@...ssion.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Jonathan Corbet <corbet@....net>,
        Dave Airlie <airlied@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Arnd Bergmann <arnd@...db.de>, Tejun Heo <tj@...nel.org>,
        Christoph Lameter <cl@...ux.com>
Subject: Re: [RFC Part1 PATCH v3 03/17] x86/mm: Secure Encrypted
 Virtualization (SEV) support

On 7/25/2017 11:28 PM, Borislav Petkov wrote:
> On Mon, Jul 24, 2017 at 02:07:43PM -0500, Brijesh Singh wrote:
>> From: Tom Lendacky <thomas.lendacky@....com>
>>
>> Provide support for Secure Encyrpted Virtualization (SEV). This initial
> 
> Your subject misses a verb and patch subjects should have an active verb
> denoting what the patch does. The sentence above is a good example.

Yup, will update.

> 
>> support defines a flag that is used by the kernel to determine if it is
>> running with SEV active.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@....com>
>> ---
>>   arch/x86/include/asm/mem_encrypt.h | 2 ++
>>   arch/x86/mm/mem_encrypt.c          | 3 +++
>>   include/linux/mem_encrypt.h        | 8 +++++++-
>>   3 files changed, 12 insertions(+), 1 deletion(-)
> 
> ...
> 
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 0fbd092..1e4643e 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -40,6 +40,9 @@ static char sme_cmdline_off[] __initdata = "off";
>>   unsigned long sme_me_mask __section(.data) = 0;
>>   EXPORT_SYMBOL_GPL(sme_me_mask);
>>   
>> +unsigned int sev_enabled __section(.data) = 0;
>> +EXPORT_SYMBOL_GPL(sev_enabled);
> 
> So sev_enabled is a pure bool used only in bool context, not like
> sme_me_mask whose value is read too. Which means, you can make the
> former static and query it only through accessor functions.

If it's made static then the sme_active()/sev_active() inline functions
would need to be turned into functions within the mem_encrypt.c file. So
there's a trade-off to do that, which is the better one?

> 
>>   /* Buffer used for early in-place encryption by BSP, no locking needed */
>>   static char sme_early_buffer[PAGE_SIZE] __aligned(PAGE_SIZE);
>>   
>> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h
>> index 1255f09..ea0831a 100644
>> --- a/include/linux/mem_encrypt.h
>> +++ b/include/linux/mem_encrypt.h
>> @@ -22,12 +22,18 @@
>>   #else	/* !CONFIG_ARCH_HAS_MEM_ENCRYPT */
>>   
>>   #define sme_me_mask	0UL
>> +#define sev_enabled	0
>>   
>>   #endif	/* CONFIG_ARCH_HAS_MEM_ENCRYPT */
>>   
>>   static inline bool sme_active(void)
>>   {
>> -	return !!sme_me_mask;
>> +	return (sme_me_mask && !sev_enabled);
> 
> You don't need the brackets. Below too.

Ok.

> 
>> +}
>> +
>> +static inline bool sev_active(void)
>> +{
>> +	return (sme_me_mask && sev_enabled);
>>   }
> 
> So this is confusing, TBH. SME and SEV are not mutually exclusive and
> yet the logic here says so. Why?
> 
> I mean, in the hypervisor context, sme_active() is still true.
> 
> /me is confused.

The kernel needs to distinguish between running under SME and running
under SEV. SME and SEV are similar but not the same. The trampoline code
is a good example.  Before paging is activated, SME will access all
memory as decrypted, but SEV will access all memory as encrypted.  So
when APs are being brought up under SME the trampoline area cannot be
encrypted, whereas under SEV the trampoline area must be encrypted.

Thanks,
Tom

>