linux-kernel - Re: [RFC PATCH v2 12/20] x86: Add support for changing memory encryption attribute

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: <4e423d15-7fe2-450a-05dd-1674bd281124@amd.com>
Date:   Mon, 12 Sep 2016 10:41:29 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Borislav Petkov <bp@...en8.de>
CC:     <linux-arch@...r.kernel.org>, <linux-efi@...r.kernel.org>,
        <kvm@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <kasan-dev@...glegroups.com>, <linux-mm@...ck.org>,
        <iommu@...ts.linux-foundation.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Arnd Bergmann <arnd@...db.de>,
        Jonathan Corbet <corbet@....net>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        Joerg Roedel <joro@...tes.org>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Ingo Molnar <mingo@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Alexander Potapenko <glider@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [RFC PATCH v2 12/20] x86: Add support for changing memory
 encryption attribute

On 09/09/2016 12:23 PM, Borislav Petkov wrote:
> On Mon, Aug 22, 2016 at 05:37:49PM -0500, Tom Lendacky wrote:
>> This patch adds support to be change the memory encryption attribute for
>> one or more memory pages.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>  arch/x86/include/asm/cacheflush.h  |    3 +
>>  arch/x86/include/asm/mem_encrypt.h |   13 ++++++
>>  arch/x86/mm/mem_encrypt.c          |   43 +++++++++++++++++++++
>>  arch/x86/mm/pageattr.c             |   75 ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 134 insertions(+)
> 
> ...
> 
>> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
>> index 72c292d..0ba9382 100644
>> --- a/arch/x86/mm/pageattr.c
>> +++ b/arch/x86/mm/pageattr.c
>> @@ -1728,6 +1728,81 @@ int set_memory_4k(unsigned long addr, int numpages)
>>  					__pgprot(0), 1, 0, NULL);
>>  }
>>  
>> +static int __set_memory_enc_dec(struct cpa_data *cpa)
>> +{
>> +	unsigned long addr;
>> +	int numpages;
>> +	int ret;
>> +
>> +	if (*cpa->vaddr & ~PAGE_MASK) {
>> +		*cpa->vaddr &= PAGE_MASK;
>> +
>> +		/* People should not be passing in unaligned addresses */
>> +		WARN_ON_ONCE(1);
> 
> Let's make this more user-friendly:
> 
> 	if (WARN_ONCE(*cpa->vaddr & ~PAGE_MASK, "Misaligned address: 0x%lx\n", *cpa->vaddr))
> 		*cpa->vaddr &= PAGE_MASK;

Will do.

> 
>> +	}
>> +
>> +	addr = *cpa->vaddr;
>> +	numpages = cpa->numpages;
>> +
>> +	/* Must avoid aliasing mappings in the highmem code */
>> +	kmap_flush_unused();
>> +	vm_unmap_aliases();
>> +
>> +	ret = __change_page_attr_set_clr(cpa, 1);
>> +
>> +	/* Check whether we really changed something */
>> +	if (!(cpa->flags & CPA_FLUSHTLB))
>> +		goto out;
>> +
>> +	/*
>> +	 * On success we use CLFLUSH, when the CPU supports it to
>> +	 * avoid the WBINVD.
>> +	 */
>> +	if (!ret && static_cpu_has(X86_FEATURE_CLFLUSH))
>> +		cpa_flush_range(addr, numpages, 1);
>> +	else
>> +		cpa_flush_all(1);
> 
> So if we fail (ret != 0) we do WBINVD unconditionally even if we don't
> have to?

Looking at __change_page_attr_set_clr() isn't it possible for some of
the pages to be changed before an error is encountered since it is
looping?  If so, we may still need to flush. The CPA_FLUSHTLB flag
should take care of a failing case where no attributes have actually
been changed.

Thanks,
Tom

> 
> Don't you want this instead:
> 
>         ret = __change_page_attr_set_clr(cpa, 1);
>         if (ret)
>                 goto out;
> 
>         /* Check whether we really changed something */
>         if (!(cpa->flags & CPA_FLUSHTLB))
>                 goto out;
> 
>         /*
>          * On success we use CLFLUSH, when the CPU supports it to
>          * avoid the WBINVD.
>          */
>         if (static_cpu_has(X86_FEATURE_CLFLUSH))
>                 cpa_flush_range(addr, numpages, 1);
>         else
>                 cpa_flush_all(1);
> 
> out:
>         return ret;
> }
> 
> ?
>