Re: [RFC PATCH v3 09/20] x86: Insure that boot memory areas are mapped properly

[Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>]

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-5C7GfCeVMHo@public.gmane.org>
> ---
>  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.

> +{
> +	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...

> +	} 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;

	...

> +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.

-- 
Regards/Gruss,
    Boris.

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