Re: [PATCH v6 3/5] x86/mm: add .data..decrypted section to hold shared variables

[Borislav Petkov]

On Fri, Sep 07, 2018 at 12:57:28PM -0500, Brijesh Singh wrote:
> kvmclock defines few static variables which are shared with the
> hypervisor during the kvmclock initialization.
> 
> When SEV is active, memory is encrypted with a guest-specific key, and
> if guest OS wants to share the memory region with hypervisor then it must

"... if *the* guest OS ... with *the* hypervisor ..."

> clear the C-bit before sharing it.

<---- newline here.

> Currently, we use
> kernel_physical_mapping_init() to split large pages before clearing the
> C-bit on shared pages. But it fails when called from the kvmclock
> initialization (mainly because memblock allocator is not ready that early

		... because *the* memblock allocator...

hmm, those definite articles are kinda all lost... :)

> during boot).
> 
> Add a __decrypted section attribute which can be used when defining
> such shared variable. The so-defined variables will be placed in the
> .data..decrypted section. This section is mapped with C=0 early
> during boot, we also ensure that the initialized values are updated
> to match with C=0 (i.e perform an in-place decryption). The
> .data..decrypted section is PMD-aligned and sized so that we avoid
> the need to split the large pages when mapping the section.
> 
> The sme_encrypt_kernel() was used to perform the in-place encryption

Here, of all things, you don't need a definite article :)

"sme_encrypt_kernel() performs the in-place encryption...

> of the Linux kernel and initrd when SME is active. The routine has been
> enhanced to decrypt the .data..decrypted section for both SME and SEV
> cases.

Otherwise, that's a much better commit message!

> Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: "Radim KrÄmÃÅ" <rkrcmar@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/mem_encrypt.h |  6 +++
>  arch/x86/kernel/head64.c           | 11 +++++
>  arch/x86/kernel/vmlinux.lds.S      | 17 +++++++
>  arch/x86/mm/mem_encrypt_identity.c | 94 ++++++++++++++++++++++++++++++++------
>  4 files changed, 113 insertions(+), 15 deletions(-)

...

> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index 8bde0a4..4cb1064 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -65,6 +65,21 @@ jiffies_64 = jiffies;
>  #define ALIGN_ENTRY_TEXT_BEGIN	. = ALIGN(PMD_SIZE);
>  #define ALIGN_ENTRY_TEXT_END	. = ALIGN(PMD_SIZE);
>  
> +/*
> + * This section contains data which will be mapped as decrypted. Memory
> + * encryption operates on a page basis. Make this section PMD-aligned
> + * to avoid spliting the pages while mapping the section early.

"splitting" - damn, that spell checker of yours is still not working... ;-\

> + *
> + * Note: We use a separate section so that only this section gets
> + * decrypted to avoid exposing more than we wish.
> + */
> +#define DATA_DECRYPTED						\
> +	. = ALIGN(PMD_SIZE);					\
> +	__start_data_decrypted = .;				\
> +	*(.data..decrypted);					\
> +	. = ALIGN(PMD_SIZE);					\
> +	__end_data_decrypted = .;				\
> +
>  #else
>  
>  #define X86_ALIGN_RODATA_BEGIN

...

> @@ -487,28 +510,69 @@ static void __init teardown_workarea_map(struct sme_workarea_data *wa,
>  	native_write_cr3(__native_read_cr3());
>  }
>  
> +static void __init decrypt_shared_data(struct sme_workarea_data *wa,
> +				       struct sme_populate_pgd_data *ppd)
> +{
> +	unsigned long decrypted_start, decrypted_end, decrypted_len;
> +
> +	/* Physical addresses of decrypted data section */
> +	decrypted_start = __pa_symbol(__start_data_decrypted);
> +	decrypted_end = ALIGN(__pa_symbol(__end_data_decrypted), PMD_PAGE_SIZE);

Why?

You have:

+       . = ALIGN(PMD_SIZE);                                    \
+       __end_data_decrypted = .;                               \

It is already aligned by definition?!?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix ImendÃrffer, Jane Smithard, Graham Norton, HRB 21284 (AG NÃrnberg)
--