| 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
| ||
|
Message-ID: <4ff5706f-367d-2204-d753-63e78931fd66@amd.com>
Date: Thu, 21 Dec 2017 10:35:37 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Borislav Petkov <bp@...en8.de>
Cc: x86@...nel.org, Brijesh Singh <brijesh.singh@....com>,
linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v1 2/3] x86/mm: Prepare sme_encrypt_kernel() for PAGE
aligned encryption
On 12/21/2017 6:58 AM, Borislav Petkov wrote:
> On Thu, Dec 07, 2017 at 05:34:02PM -0600, Tom Lendacky wrote:
>> In preparation for encrypting more than just the kernel, the encryption
>> support in sme_encrypt_kernel() needs to support 4KB page aligned
>> encryption instead of just 2MB large page aligned encryption.
>
> ...
>
>> static void __init __sme_map_range(pgd_t *pgd, unsigned long vaddr,
>> unsigned long vaddr_end,
>> - unsigned long paddr, pmdval_t pmd_flags)
>> + unsigned long paddr,
>> + pmdval_t pmd_flags, pteval_t pte_flags)
>> {
>> - while (vaddr < vaddr_end) {
>> - sme_populate_pgd(pgd, vaddr, paddr, pmd_flags);
>> + if (vaddr & ~PMD_PAGE_MASK) {
>> + /* Start is not 2MB aligned, create PTE entries */
>> + unsigned long pmd_start = ALIGN(vaddr, PMD_PAGE_SIZE);
>> +
>> + while (vaddr < pmd_start) {
>> + sme_populate_pgd(pgd, vaddr, paddr, pte_flags);
>> +
>> + vaddr += PAGE_SIZE;
>> + paddr += PAGE_SIZE;
>> + }
>> + }
>
> This chunk...
>
>> +
>> + while (vaddr < (vaddr_end & PMD_PAGE_MASK)) {
>> + sme_populate_pgd_large(pgd, vaddr, paddr, pmd_flags);
>>
>> vaddr += PMD_PAGE_SIZE;
>> paddr += PMD_PAGE_SIZE;
>> }
>> +
>> + if (vaddr_end & ~PMD_PAGE_MASK) {
>> + /* End is not 2MB aligned, create PTE entries */
>> + while (vaddr < vaddr_end) {
>> + sme_populate_pgd(pgd, vaddr, paddr, pte_flags);
>> +
>> + vaddr += PAGE_SIZE;
>> + paddr += PAGE_SIZE;
>> + }
>> + }
>
> ... and this chunk look like could be extracted in a wrapper as they're
> pretty similar.
>
Should be doable, I'll take care of it.
>> static void __init sme_map_range_encrypted(pgd_t *pgd,
>> @@ -582,7 +658,8 @@ static void __init sme_map_range_encrypted(pgd_t *pgd,
>> unsigned long vaddr_end,
>> unsigned long paddr)
>> {
>> - __sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_ENC);
>> + __sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> + PMD_FLAGS_ENC, PTE_FLAGS_ENC);
>> }
>>
>> static void __init sme_map_range_decrypted(pgd_t *pgd,
>> @@ -590,7 +667,8 @@ static void __init sme_map_range_decrypted(pgd_t *pgd,
>> unsigned long vaddr_end,
>> unsigned long paddr)
>> {
>> - __sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_DEC);
>> + __sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> + PMD_FLAGS_DEC, PTE_FLAGS_DEC);
>> }
>>
>> static void __init sme_map_range_decrypted_wp(pgd_t *pgd,
>> @@ -598,19 +676,20 @@ static void __init sme_map_range_decrypted_wp(pgd_t *pgd,
>> unsigned long vaddr_end,
>> unsigned long paddr)
>> {
>> - __sme_map_range(pgd, vaddr, vaddr_end, paddr, PMD_FLAGS_DEC_WP);
>> + __sme_map_range(pgd, vaddr, vaddr_end, paddr,
>> + PMD_FLAGS_DEC_WP, PTE_FLAGS_DEC_WP);
>> }
>>
>> static unsigned long __init sme_pgtable_calc(unsigned long len)
>> {
>> - unsigned long p4d_size, pud_size, pmd_size;
>> + unsigned long p4d_size, pud_size, pmd_size, pte_size;
>> unsigned long total;
>>
>> /*
>> * Perform a relatively simplistic calculation of the pagetable
>> - * entries that are needed. That mappings will be covered by 2MB
>> - * PMD entries so we can conservatively calculate the required
>> - * number of P4D, PUD and PMD structures needed to perform the
>> + * entries that are needed. That mappings will be covered mostly
>
> Those
>
Got it.
>> + * by 2MB PMD entries so we can conservatively calculate the required
>> + * number of P4D, PUD, PMD and PTE structures needed to perform the
>> * mappings. Incrementing the count for each covers the case where
>> * the addresses cross entries.
>> */
>> @@ -626,8 +705,9 @@ static unsigned long __init sme_pgtable_calc(unsigned long len)
>> }
>> pmd_size = (ALIGN(len, PUD_SIZE) / PUD_SIZE) + 1;
>> pmd_size *= sizeof(pmd_t) * PTRS_PER_PMD;
>> + pte_size = 2 * sizeof(pte_t) * PTRS_PER_PTE;
>
> This needs the explanation from the commit message about the 2 extra
> pages, as a comment above it.
Yup, I'll include the info about the (possible) extra PTE entries.
>
>> - total = p4d_size + pud_size + pmd_size;
>> + total = p4d_size + pud_size + pmd_size + pte_size;
>>
>> /*
>> * Now calculate the added pagetable structures needed to populate
>> @@ -710,10 +790,13 @@ void __init sme_encrypt_kernel(void)
>>
>> /*
>> * The total workarea includes the executable encryption area and
>> - * the pagetable area.
>> + * the pagetable area. The start of the workarea is already 2MB
>> + * aligned, align the end of the workarea on a 2MB boundary so that
>> + * we don't try to create/allocate PTE entries from the workarea
>> + * before it is mapped.
>> */
>> workarea_len = execute_len + pgtable_area_len;
>> - workarea_end = workarea_start + workarea_len;
>> + workarea_end = ALIGN(workarea_start + workarea_len, PMD_PAGE_SIZE);
>>
>> /*
>> * Set the address to the start of where newly created pagetable
>> diff --git a/arch/x86/mm/mem_encrypt_boot.S b/arch/x86/mm/mem_encrypt_boot.S
>> index 730e6d5..20cca86 100644
>> --- a/arch/x86/mm/mem_encrypt_boot.S
>> +++ b/arch/x86/mm/mem_encrypt_boot.S
>> @@ -120,23 +120,33 @@ ENTRY(__enc_copy)
>>
>> wbinvd /* Invalidate any cache entries */
>>
>> + push %r12
>> +
>> /* Copy/encrypt 2MB at a time */
>> + movq $PMD_PAGE_SIZE, %r12
>> 1:
>> + cmpq %r12, %r9
>> + jnb 2f
>> + movq %r9, %r12
>> +
>> +2:
>> movq %r11, %rsi /* Source - decrypted kernel */
>> movq %r8, %rdi /* Dest - intermediate copy buffer */
>> - movq $PMD_PAGE_SIZE, %rcx /* 2MB length */
>> + movq %r12, %rcx
>> rep movsb
>>
>> movq %r8, %rsi /* Source - intermediate copy buffer */
>> movq %r10, %rdi /* Dest - encrypted kernel */
>> - movq $PMD_PAGE_SIZE, %rcx /* 2MB length */
>> + movq %r12, %rcx
>> rep movsb
>>
>> - addq $PMD_PAGE_SIZE, %r11
>> - addq $PMD_PAGE_SIZE, %r10
>> - subq $PMD_PAGE_SIZE, %r9 /* Kernel length decrement */
>> + addq %r12, %r11
>> + addq %r12, %r10
>> + subq %r12, %r9 /* Kernel length decrement */
>> jnz 1b /* Kernel length not zero? */
>>
>> + pop %r12
>> +
>> /* Restore PAT register */
>> push %rdx /* Save original PAT value */
>> movl $MSR_IA32_CR_PAT, %ecx
>
> Right, for this here can you pls do a cleanup pre-patch which does
>
> push
> push
> push
>
> /* meat of the function */
>
> pop
> pop
> pop
>
> as now those pushes and pops are interspersed with the rest of the insns
> and that makes following through it harder. F17h has a stack engine so
> those streams of pushes and pops won't hurt perf and besides, this is
> not even perf-sensitive.
>
Ok, I'll clean that up to move any pushes/pops to the beginning/end of
the function, as well as moving some register saving earlier which may
eliminate some push/pop instructions.
Thanks,
Tom
> Thx.
>
Powered by blists - more mailing lists