linux-kernel - Re: [PATCH Part1 RFC v2 14/20] x86/sev: Add helper for validating pages in early enc attribute changes

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:   Wed, 26 May 2021 08:34:37 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        tglx@...utronix.de, jroedel@...e.de, thomas.lendacky@....com,
        pbonzini@...hat.com, mingo@...hat.com, dave.hansen@...el.com,
        rientjes@...gle.com, seanjc@...gle.com, peterz@...radead.org,
        hpa@...or.com, tony.luck@...el.com
Subject: Re: [PATCH Part1 RFC v2 14/20] x86/sev: Add helper for validating
 pages in early enc attribute changes


On 5/26/21 5:39 AM, Borislav Petkov wrote:
> On Fri, Apr 30, 2021 at 07:16:10AM -0500, Brijesh Singh wrote:
>> +static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool validate)
>> +{
>> +	unsigned long vaddr_end;
>> +	int rc;
>> +
>> +	vaddr = vaddr & PAGE_MASK;
>> +	vaddr_end = vaddr + (npages << PAGE_SHIFT);
>> +
>> +	while (vaddr < vaddr_end) {
>> +		rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
>> +		if (rc) {
>> +			WARN(rc, "Failed to validate address 0x%lx ret %d", vaddr, rc);
> WARN does return the condition it warned on, look at its definition.
>
> IOW, you can do (and btw e_fail is not needed either):
>
>                 rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
>                 if (WARN(rc, "Failed to validate address 0x%lx ret %d", vaddr, rc))
> 			sev_es_terminate(1, GHCB_TERM_PVALIDATE);
>
> Ditto for the other WARN.

Okay, I agree with comment, I will go through each code block and use
the return value from the WARN().

..
>> +	unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>> +
>> +	if (dec) {
>> +		/* If the paddr needs to be accessed decrypted, make the page shared before memcpy. */
>> +		early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages);
>> +
>> +		memcpy(dst, src, sz);
>> +
>> +		/* Restore the page state after the memcpy. */
>> +		early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages);
>> +	} else {
>> +		memcpy(dst, src, sz);
>> +	}
> Hmm, this function needs reorg. How about this:

I think with your edits its looking similar to what I had in v1. In v1
early_snp_set_memory_shared() was not doing sev_snp_active()  check
whereas it does now.I didn't check the sev_snp_active() in this function
but I agree that it can be reorg. I am good with your changes. thanks


>
> /*
>  * When SNP is active, change the page state from private to shared before
>  * copying the data from the source to destination and restore after the copy.
>  * This is required because the source address is mapped as decrypted by the
>  * caller of the routine.
>  */
> static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
>                                      unsigned long paddr, bool decrypt)
> {
>         unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>
>         if (!sev_snp_active() || !decrypt) {
>                 memcpy(dst, src, sz);
>                 return;
> 	}
>
>         /*
>          * If the paddr needs to be accessed decrypted, mark the page
>          * shared in the RMP table before copying it.
>          */
>         early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages);
>
>         memcpy(dst, src, sz);
>
>         /* Restore the page state after the memcpy. */
>         early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages);
> }
>
>