linux-kernel - Re: [PATCH v2 3/5] x86/mm: Centralize PMD flags in sme_encrypt

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]

Date:   Mon, 8 Jan 2018 17:18:26 -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 v2 3/5] x86/mm: Centralize PMD flags in
 sme_encrypt_kernel()

On 1/7/2018 10:28 AM, Borislav Petkov wrote:
> On Thu, Dec 21, 2017 at 04:03:12PM -0600, Tom Lendacky wrote:
>> In preparation for encrypting more than just the kernel during early
>> boot processing, centralize the use of the PMD flag settings based
>> on the type of mapping desired.  When 4KB aligned encryption is added,
>> this will allow either PTE flags or large page PMD flags to be used
>> without requiring the caller to adjust.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
>> ---
>>  arch/x86/mm/mem_encrypt.c |  131 ++++++++++++++++++++++++++-------------------
>>  1 file changed, 77 insertions(+), 54 deletions(-)
>>
>> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
>> index 5a20696..9b180f8 100644
>> --- a/arch/x86/mm/mem_encrypt.c
>> +++ b/arch/x86/mm/mem_encrypt.c
>> @@ -468,31 +468,40 @@ struct sme_populate_pgd_data {
>>  	void	*pgtable_area;
>>  	pgd_t	*pgd;
>>  
>> -	pmdval_t pmd_val;
>> +	pmdval_t pmd_flags;
>> +	unsigned long paddr;
>> +
>>  	unsigned long vaddr;
>> +	unsigned long vaddr_end;
>>  };
>>  
>> -static void __init sme_clear_pgd(pgd_t *pgd_base, unsigned long start,
>> -				 unsigned long end)
>> +static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
>>  {
>>  	unsigned long pgd_start, pgd_end, pgd_size;
>>  	pgd_t *pgd_p;
>>  
>> -	pgd_start = start & PGDIR_MASK;
>> -	pgd_end = end & PGDIR_MASK;
>> +	pgd_start = ppd->vaddr & PGDIR_MASK;
>> +	pgd_end = ppd->vaddr_end & PGDIR_MASK;
>>  
>>  	pgd_size = (((pgd_end - pgd_start) / PGDIR_SIZE) + 1);
>>  	pgd_size *= sizeof(pgd_t);
> 
> This is a strange way of writing this. I'd expect:
> 
> 	pgd_size = (((pgd_end - pgd_start) / PGDIR_SIZE) + 1) * sizeof(pgd_t);

Yup, clearer as one line.  I can fix that up.

> 
>>  
>> -	pgd_p = pgd_base + pgd_index(start);
>> +	pgd_p = ppd->pgd + pgd_index(ppd->vaddr);
>>  
>>  	memset(pgd_p, 0, pgd_size);
>>  }
>>  
>> -#define PGD_FLAGS	_KERNPG_TABLE_NOENC
>> -#define P4D_FLAGS	_KERNPG_TABLE_NOENC
>> -#define PUD_FLAGS	_KERNPG_TABLE_NOENC
>> -#define PMD_FLAGS	(__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>> +#define PGD_FLAGS		_KERNPG_TABLE_NOENC
>> +#define P4D_FLAGS		_KERNPG_TABLE_NOENC
>> +#define PUD_FLAGS		_KERNPG_TABLE_NOENC
>> +
>> +#define PMD_FLAGS_LARGE		(__PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL)
>> +
>> +#define PMD_FLAGS_DEC		PMD_FLAGS_LARGE
>> +#define PMD_FLAGS_DEC_WP	((PMD_FLAGS_DEC & ~_PAGE_CACHE_MASK) | \
>> +				 (_PAGE_PAT | _PAGE_PWT))
>> +
>> +#define PMD_FLAGS_ENC		(PMD_FLAGS_LARGE | _PAGE_ENC)
>>  
>>  static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
>>  {
>> @@ -561,7 +570,36 @@ static void __init sme_populate_pgd_large(struct sme_populate_pgd_data *ppd)
>>  
>>  	pmd_p += pmd_index(ppd->vaddr);
>>  	if (!native_pmd_val(*pmd_p) || !(native_pmd_val(*pmd_p) & _PAGE_PSE))
>> -		native_set_pmd(pmd_p, native_make_pmd(ppd->pmd_val));
>> +		native_set_pmd(pmd_p,
>> +			       native_make_pmd(ppd->paddr | ppd->pmd_flags));
> 
> Never do those ugly line breaks. Just let it stick out.

Will do.

Thanks,
Tom

> 
> Otherwise, sme_encrypt_kernel() is starting to look quite readable :)
>