linux-kernel - Re: [PATCH Part1 RFC v3 21/22] x86/sev: Register SNP guest request platform device

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]

Message-ID: <aef906ea-764d-0bbc-49c6-b3ecfc192214@amd.com>
Date:   Mon, 14 Jun 2021 08:20:16 -0500
From:   Brijesh Singh <brijesh.singh@....com>
To:     "Dr. David Alan Gilbert" <dgilbert@...hat.com>
Cc:     brijesh.singh@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        linux-crypto@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        Tom Lendacky <thomas.lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>, tony.luck@...el.com,
        npmccallum@...hat.com
Subject: Re: [PATCH Part1 RFC v3 21/22] x86/sev: Register SNP guest request
 platform device

I see that Tom answered few comments. I will cover others.


On 6/9/21 2:24 PM, Dr. David Alan Gilbert wrote:
+ /*
>> +	 * The message sequence counter for the SNP guest request is a 64-bit value
>> +	 * but the version 2 of GHCB specification defines the 32-bit storage for the
>> +	 * it.
>> +	 */
>> +	if ((count + 1) >= INT_MAX)
>> +		return 0;
> Is that UINT_MAX?

Good catch. It should be UINT_MAX.


> +	/*
> +	 * The secret page contains the VM encryption key used for encrypting the
> +	 * messages between the guest and the PSP. The secrets page location is
> +	 * available either through the setup_data or EFI configuration table.
> +	 */
> +	if (hdr->cc_blob_address) {
> +		paddr = hdr->cc_blob_address;
> Can you trust the paddr the host has given you or do you need to do some
> form of validation?
The paddr is mapped encrypted. That means that data  in the paddr must
be encrypted either through the guest or PSP. After locating the paddr,
we perform a simply sanity check (32-bit magic string "AMDE"). See the
verify header check below. Unfortunately the secrets page itself does
not contain any magic key which we can use to ensure that
hdr->secret_paddr is actually pointing to the secrets pages but all of
these memory is accessed encrypted so its safe to access it. If VMM
lying to us that basically means guest will not be able to communicate
with the PSP and can't do the attestation etc.

>
> Dave
> +	} else if (efi_enabled(EFI_CONFIG_TABLES)) {
> +#ifdef CONFIG_EFI
> +		paddr = cc_blob_phys;
> +#else
> +		return -ENODEV;
> +#endif
> +	} else {
> +		return -ENODEV;
> +	}
> +
> +	info = memremap(paddr, sizeof(*info), MEMREMAP_WB);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	/* Verify the header that its a valid SEV_SNP CC header */
> +	if ((info->magic == CC_BLOB_SEV_HDR_MAGIC) &&
> +	    info->secrets_phys &&
> +	    (info->secrets_len == PAGE_SIZE)) {
> +		res->start = info->secrets_phys;
> +		res->end = info->secrets_phys + info->secrets_len;
> +		res->flags = IORESOURCE_MEM;
> +		snp_secrets_phys = info->secrets_phys;
> +		ret = 0;
> +	}
> +
> +	memunmap(info);
> +	return ret;
> +}
> +