From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.9766.1623142175938951859 for ; Tue, 08 Jun 2021 01:49:36 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=EAV8tYgm; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623142175; 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=VzNkkNREMkCdEJ+oMMzwnS3lDiFuh6M46Co4+pe++So=; b=EAV8tYgmqlsX3IcET40zT30RVdN8Yvb5FW10rlZcfYNXFeS0oedAWBIoO/viiqpD0HlTYU PJ1pr3pwRGpVXEbwur0fcnTNDRXwwlQtEZeLH6hI8UxtnLVHOwfIA0dRPhXryUpr3O785D 0/g6m+T1tzCCT8Uc60T0AAUA91HoVNQ= 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-572-SbD8Kr30PJupCWibWp27lw-1; Tue, 08 Jun 2021 04:49:31 -0400 X-MC-Unique: SbD8Kr30PJupCWibWp27lw-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 4332310074A6; Tue, 8 Jun 2021 08:49:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-27.ams2.redhat.com [10.36.113.27]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 340A0620DE; Tue, 8 Jun 2021 08:49:27 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v3 04/22] OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features To: Brijesh Singh Cc: devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , Ard Biesheuvel References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-5-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <0744c84d-ca70-1fe6-a1c0-b97bb87affc5@redhat.com> Date: Tue, 8 Jun 2021 10:49:25 +0200 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: 8bit On 06/07/21 15:37, Brijesh Singh wrote: > Also, I was trying to avoid the cases where the malicious hypervisor > changing the feature value after the GHCB negotiation is completed.  > e.g, during the reset vector they give us one feature value and change > them during SEC or PEI or DXE instances run. They can't break the > security of SNP by doing so, but I thought its good to avoid querying > the features on every phase. If there is *feasible* security problem (attack), then my "information flow purity" criteria are irrelevant. Security trumps all. My understanding is though (per your explanation above) that there is no real security problem here. Furthermore, we're not going to query the feature set in every phase. We're going to set the PCDs in the PEI phase; there shouldn't be hardware querying in the DXE phase. That's quite standard business in OVMF. >> Instead, we should have a new MemEncryptSevLib function that outputs the FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(), but it should return a RETURN_STATUS, and produce the FEATURES bitmask through an output parameter. > > This is one of thing which I noticed last week, we are actually creating > a circular dependency. The MemEncryptSevLib provides the routines which > are used by the VmgExitLib. The MemEncryptSevLib should *not* depend on > the VmgExitLib. If we extend the MemEncryptSevLib to provide a routine > to query the features then we will be required to include the > VmgExitLib. The patch #13 extends the MemEncryptSevLib to provide > functions for the page validation and it requires the VmgExitLib. I am > trying to see what I can do to not create this circular dependency and > still keep code reuse. As far as I remember, a circular dependency is only a problem if both library instances in question have constructors. If a library instance needs no construction (has no constructor), then it does not partake in the topological sorting (for initialization) performed by BaseTools. In this case, at the end of your RFCv3 series (minus patch#21), no INF file in either "OvmfPkg/Library/BaseMemEncryptSevLib" or "OvmfPkg/Library/VmgExitLib" seems to specify a CONSTRUCTOR, so purely from the build perspective, there should be no issue. But, I have another idea here: >> The SEC instance of the function should return RETURN_UNSUPPORTED. >> >> The PEI instance should use the GHCB MSR protocol, with the help of the AsmCpuId(), AsmWriteMsr64(), AsmReadMsr64() and AsmVmgExit() BaseLib functions. >> >> The DXE instance should read back the PCD. the above basically tells us that this library API would be a single-use interface. It wouldn't work in SEC, it would do actual work in PEI (namely, in PlatformPei), and it wouldn't do any "real work" in DXE. To me, the boundary is not crystal clear, but the above struggles *suggest* that we might not want this API to be a MemEncryptSevLib function (or any library function) at all. If abstracting out the API causes more pain than it does good, then let's just not abstract it. Meaning, you could open-code the fetching of features (using VmgExitLib) in PlatormPei, set the PCD, and be done with it. The only place where the PCD (PcdGhcbHypervisorFeatures) is actually used (as far as I can see) is patch#21, in UefiCpuPkg. We could reasonably say that the "real API", namely the declaration of the PCD, *already exists*, namely in the "UefiCpuPkg/UefiCpuPkg.dec" file, from your commit e6994b1d5226 ("UefiCpuPkg: Define the SEV-SNP specific dynamic PCDs", 2021-06-04). There are other examples where a cross-package PCD is set once and for all in OvmfPkg/PlatformPei (random example: "PcdCpuBootLogicalProcessorNumber"). We don't have to turn everything into a lib class API, especially if it causes more pain than help. ... But maybe I just need to accept that we have to repurpose SEC_SEV_ES_WORK_AREA, considering it a super-early "HOB list" of sorts. Same as the PEI phase is considered the "HOB producer phase", outputting a bunch of disparate bits of info, we could consider the SEV-ES parts of the Reset Vector such an "early info bits" producer phase. I think this is a very big conceptual step away from the original purpose of SEC_SEV_ES_WORK_AREA (note the *name* of the structure: "work area"! HOBs are not "work areas", they are effectively read-only, once produced). But perhaps this is what we need (and then with proper documentation). NB however that HOBs have types, GUIDed HOBs have GUIDs, the HOB types are specified in PI, and GUIDs are expressly declared to stand for various purposes at least in edk2 DEC files. All that helps with discerning the information flow. So... I'd still prefer keeping SEC_SEV_ES_WORK_AREA as minimal as possible. Tom, any comments? Thank you Brijesh for raising great points! Laszlo