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: <d6e8539a-0e4d-8d16-ffe1-e88595cb7ab3@amd.com>
Date:   Sat, 19 Nov 2016 12:12:27 -0600
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>, Rik van Riel <riel@...hat.com>,
        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>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Larry Woodman <lwoodman@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [RFC PATCH v3 09/20] x86: Insure that boot memory areas are
 mapped properly

On 11/17/2016 6:20 AM, Borislav Petkov wrote:
> On Wed, Nov 09, 2016 at 06:36:20PM -0600, Tom Lendacky wrote:
>> The boot data and command line data are present in memory in an
>> un-encrypted state and are copied early in the boot process.  The early
>> page fault support will map these areas as encrypted, so before attempting
>> to copy them, add unencrypted mappings so the data is accessed properly
>> when copied.
>>
>> For the initrd, encrypt this data in place. Since the future mapping of the
>> initrd area will be mapped as encrypted the data will be accessed properly.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>  arch/x86/include/asm/mem_encrypt.h |   13 ++++++++
>>  arch/x86/kernel/head64.c           |   21 ++++++++++++--
>>  arch/x86/kernel/setup.c            |    9 ++++++
>>  arch/x86/mm/mem_encrypt.c          |   56 ++++++++++++++++++++++++++++++++++++
>>  4 files changed, 96 insertions(+), 3 deletions(-)
> 
> ...
> 
>> @@ -122,6 +131,12 @@ static void __init copy_bootdata(char *real_mode_data)
>>  	char * command_line;
>>  	unsigned long cmd_line_ptr;
>>  
>> +	/*
>> +	 * If SME is active, this will create un-encrypted mappings of the
>> +	 * boot data in advance of the copy operations
> 						      ^
> 						      |
> 					    Fullstop--+
> 
>> +	 */
>> +	sme_map_bootdata(real_mode_data);
>> +
>>  	memcpy(&boot_params, real_mode_data, sizeof boot_params);
>>  	sanitize_boot_params(&boot_params);
>>  	cmd_line_ptr = get_cmd_line_ptr();
> 
> ...
> 
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 06235b4..411210d 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -16,8 +16,11 @@
>>  
>>  #include <asm/tlbflush.h>
>>  #include <asm/fixmap.h>
>> +#include <asm/setup.h>
>> +#include <asm/bootparam.h>
>>  
>>  extern pmdval_t early_pmd_flags;
>> +int __init __early_make_pgtable(unsigned long, pmdval_t);
>>  
>>  /*
>>   * Since sme_me_mask is set early in the boot process it must reside in
>> @@ -126,6 +129,59 @@ void __init sme_early_mem_dec(resource_size_t paddr, unsigned long size)
>>  	}
>>  }
>>  
>> +static void __init *sme_bootdata_mapping(void *vaddr, unsigned long size)
> 
> So this could be called __sme_map_bootdata(). "sme_bootdata_mapping"
> doesn't tell me what the function does as there's no verb in the name.
> 

Ok, makes sense.

>> +{
>> +	unsigned long paddr = (unsigned long)vaddr - __PAGE_OFFSET;
>> +	pmdval_t pmd_flags, pmd;
>> +	void *ret = vaddr;
> 
> That *ret --->
> 
>> +
>> +	/* Use early_pmd_flags but remove the encryption mask */
>> +	pmd_flags = early_pmd_flags & ~sme_me_mask;
>> +
>> +	do {
>> +		pmd = (paddr & PMD_MASK) + pmd_flags;
>> +		__early_make_pgtable((unsigned long)vaddr, pmd);
>> +
>> +		vaddr += PMD_SIZE;
>> +		paddr += PMD_SIZE;
>> +		size = (size < PMD_SIZE) ? 0 : size - PMD_SIZE;
> 
> 			size <= PMD_SIZE
> 
> 				looks more obvious to me...

Ok, will do.

> 
>> +	} while (size);
>> +
>> +	return ret;
> 
> ---> is simply passing vaddr out. So the function can be just as well be
> void and you can do below:
> 
> 	__sme_map_bootdata(real_mode_data, sizeof(boot_params));
> 
> 	boot_data = (struct boot_params *)real_mode_data;
> 
> 	...

Ok, that simplifies the function too.

> 
>> +void __init sme_map_bootdata(char *real_mode_data)
>> +{
>> +	struct boot_params *boot_data;
>> +	unsigned long cmdline_paddr;
>> +
>> +	if (!sme_me_mask)
>> +		return;
>> +
>> +	/*
>> +	 * The bootdata will not be encrypted, so it needs to be mapped
>> +	 * as unencrypted data so it can be copied properly.
>> +	 */
>> +	boot_data = sme_bootdata_mapping(real_mode_data, sizeof(boot_params));
>> +
>> +	/*
>> +	 * Determine the command line address only after having established
>> +	 * the unencrypted mapping.
>> +	 */
>> +	cmdline_paddr = boot_data->hdr.cmd_line_ptr |
>> +			((u64)boot_data->ext_cmd_line_ptr << 32);
> 
> <---- newline here.
> 
>> +	if (cmdline_paddr)
>> +		sme_bootdata_mapping(__va(cmdline_paddr), COMMAND_LINE_SIZE);
>> +}
>> +
>> +void __init sme_encrypt_ramdisk(resource_size_t paddr, unsigned long size)
>> +{
>> +	if (!sme_me_mask)
>> +		return;
>> +
>> +	sme_early_mem_enc(paddr, size);
>> +}
> 
> So this one could simply be called sme_encrypt_area() and be used for
> other things. There's nothing special about encrypting a ramdisk, by the
> looks of it.

The sme_early_mem_enc() function is already exposed so I'll use that. I
originally had it that way but tried to hide any logic associated with
it by just calling this function.  Any changes in logic in the future
would be handled within the SME function.  But that can be done in the
future if needed.

Thanks,
Tom

> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ