From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.10505.1607591539946664810 for ; Thu, 10 Dec 2020 01:12:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=NI2U90WX; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607591539; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=P4avAVnFUSqxNSdre4CZ8vz8RgfPBkIFLWnmuPY3Cdw=; b=NI2U90WXqFTSeuqytMUX+vE+GUmsWeWEelRi8fck7cVDI6Q2HCQ9JNejdBUgLZATZUI2gG cNNC1WU8A3bNPLbFXfeKR9fiU8F8bY+M7Nq9991Eawfa5QjU967Ihku8qTsU/cnUYw827t D+Towvb/jPJ0WOT+3nJ25DhdEmXn2Ls= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-181-l6h8LUyOMtOfHSESjWZFtQ-1; Thu, 10 Dec 2020 04:12:15 -0500 X-MC-Unique: l6h8LUyOMtOfHSESjWZFtQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9FBB6800D55; Thu, 10 Dec 2020 09:12:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-52.ams2.redhat.com [10.36.113.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9BC512BFE9; Thu, 10 Dec 2020 09:12:08 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 6/6] OvmfPkg/AmdSev: Expose the Sev Secret area using a configuration table To: "Yao, Jiewen" , "devel@edk2.groups.io" , "jejb@linux.ibm.com" Cc: "dovmurik@linux.vnet.ibm.com" , "Dov.Murik1@il.ibm.com" , "ashish.kalra@amd.com" , "brijesh.singh@amd.com" , "tobin@ibm.com" , "david.kaplan@amd.com" , "jon.grimm@amd.com" , "thomas.lendacky@amd.com" , "frankeh@us.ibm.com" , "Dr . David Alan Gilbert" , "Justen, Jordan L" , Ard Biesheuvel References: <20201130202819.3910-1-jejb@linux.ibm.com> <20201130202819.3910-7-jejb@linux.ibm.com> From: "Laszlo Ersek" Message-ID: <35757c6b-4728-e58e-39f3-f055ec1beba2@redhat.com> Date: Thu, 10 Dec 2020 10:12:07 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Jiewen, On 12/09/20 13:02, Yao, Jiewen wrote: > Hi James > I am not sure if this solution is only for AMD SEV or it is a generic solution to "pass a secret to grub and let grub decrypt the disk". > > If it is only for AMD SEV, please stop reading and ignore my comment below. > > If it is designed to be a generic solution to pass a secret to grub and let grub decrypt the disk. I have some thought below: > Intel TDX (https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html) have similar feature - a TDX virtual firmware may do attestation and get a key from remote key server. > We might use same architecture to pass the secrete to grub. > Initially, we define an ACPI 'SVKL' table to pass the secrete in intel-tdx-guest-hypervisor-communication-interface (https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf), section 4.4 storage volume key data. > But it is also OK if you want to use UEFI configuration table. > > If we need a common API for both AMD SEV and Intel TDX, then I recommend some enhancement for SevLaunchSecret.h. > 1) The file name (SevLaunchSecret.h) should be generic, such as TrustedVmSecret, StorageVolumeKey, etc. It should not include 'SEV'. Otherwise, we have to define a new GUID for 'TDX'. > 2) The GUID name (gSevLaunchSecretGuid, SEV_LAUNCH_SECRET_GUID) should be generic. Same reason above. > 3) The data structure name (SEV_LAUNCH_SECRET_LOCATION) should be generic. Same reason above. > 4) The data structure field (SEV_LAUNCH_SECRET_LOCATION.Base) should use UINTN or EFI_PHYSICAL_ADDRESS to support above 4GB memory location. > 5) The internal data structure of the secret is not defined. Is it raw binary? Or ASCII string password? Or DER format certificate? Or PEM format key? At least, we shall describe it in the header file. > 6) The might be a chance that a key server need input multiple keys to a trusted VM. How we handle this? Do we expect multiple UEFI configuration tables and each table support one key? or one table to support multiple keys? > > Would you please take a look at intel-tdx-guest-hypervisor-communication-interface, section 4.4 storage volume key data. > We defined multiple key layout, key type and key format. Please let us know if you have any thought. These are several change requests. I do not disagree with them, but I strongly propose we implement them separately. James's current v3 series presents a state that has been tested and reviewed. I'd like to commit it as-is. If for nothing else, then because I'd like the edk2 git commit history to have a snapshot of the work at this stage. We have ample time until the next edk2 stable tag. Right after this v3 series is committed, you guys can start generalizing it (e.g., renaming files and variables). Working from special case to general case is not uncommon. The feature need not be ready as soon as it is committed; it needs to be stable and externally compatible at the next stable tag. If some changes from your list above would be incompatible with other software (which is also in the making, to my understanding), then I would request / propose that patches for those other projects be held back, until the generalization of the edk2 patches reaches a certain maturity. Basically, I wouldn't like to do an incremental review on this series that introduces the above-described *amount* of changes, from v3 to v4. And I would like to perform a from-the-scratch review even less, on this series. I believe your requests have merit, I'd just like to see patches "on top" that implement them. We're right after the last stable tag, this is the time for some incompatibility, until things settle. Thanks Laszlo