From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (NAM11-CO1-obe.outbound.protection.outlook.com [40.107.220.42]) by mx.groups.io with SMTP id smtpd.web09.1.1623163818367627820 for ; Tue, 08 Jun 2021 07:50:18 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@amd.com header.s=selector1 header.b=wLrkHMwB; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.220.42, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=D3Ew0allxEp9oYC4ZvuKfwPyjvfWIIucNhoiUZ9QYqiVd1CuAGy12X3siHUWD+rOjDEZkcYdFxDMx/iDQUB3+EPyAOHswo7GJOUSLUfXs6ESWlFwxEQ0w8KsWlPDQ0xKyaH32VxteqrUiXwWu/p64PoCMOAP7WZTT1O2iqYHdlQ7Hq1iwuql8/8Eyo90vjsYFh3B2X2WT1wwBcYnfCnozDugLrMg4DYQK10hGN0lmLHCYqrQ4zzB8E8jvEPokht1JvO4f7K2Jvk5jod46Pa3J0zSLRbvv79gg5K7CbFoj1ibagZmbJaMtZVMYWOTDb28fWEoMDKSNNrjRrSymVcRfQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=c86XYuNWpqcOByBDX2doy4kVtsoualyYxvH8So2JXbg=; b=GhuTEndqzd5kSTxbdOgkbdb1dxepPAC+M5OYA/8uCKwOSG4tgwx2h1wjTH7oSRq6z3WG6aUgp6X6v6dTOtR1bprucTOnTagTb+WPL11pIaRWy0EMA2IjodX5Yc+WSr+zF1jMBDy4KmrmGCgK7+U+14ECfITli2A3zIX/s8pIaD1JtipiTFBgQhlrQmNVXM+glgfj8rnQ4XmHMfurZUCY9mRfzZb6yQik7NyBdWnuXvFVvdhIzRiVdH4vE5uTVB+AnujgXDGuBEhJkAfK6iP3tWyV5vAC/4N70vNJCBpowy9AyOlXXtn8rjhodEayBKE9tjfE5jHWSxAnaxDAGMRAHg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=c86XYuNWpqcOByBDX2doy4kVtsoualyYxvH8So2JXbg=; b=wLrkHMwBp0xH5Xz85IIAgF7kXm0kzPhgJQzK4nlH55vzIT65FzQ5bKqcjr1Aku+1cGmrxlQagmDczdFlsO2SMUIph1qHRYA7sAIjMZrjkX8kOi6pphvdptiwkPIWNif7y83eJR4KyJ/ITBjQTqRq7P6kh0bktXm/NW2wyqXAZpA= Authentication-Results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=amd.com; Received: from SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) by SA0PR12MB4509.namprd12.prod.outlook.com (2603:10b6:806:9e::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4195.24; Tue, 8 Jun 2021 14:50:16 +0000 Received: from SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::a8a9:2aac:4fd1:88fa]) by SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::a8a9:2aac:4fd1:88fa%3]) with mapi id 15.20.4219.021; Tue, 8 Jun 2021 14:50:16 +0000 CC: brijesh.singh@amd.com, devel@edk2.groups.io, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Erdem Aktas , Eric Dong , Ray Ni , Rahul Kumar , Ard Biesheuvel Subject: Re: [edk2-devel] [PATCH RFC v3 04/22] OvmfPkg/MemEncryptSevLib: extend Es Workarea to include hv features To: Laszlo Ersek References: <20210526231118.12946-1-brijesh.singh@amd.com> <20210526231118.12946-5-brijesh.singh@amd.com> <0744c84d-ca70-1fe6-a1c0-b97bb87affc5@redhat.com> From: "Brijesh Singh" Message-ID: Date: Tue, 8 Jun 2021 09:50:14 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 In-Reply-To: <0744c84d-ca70-1fe6-a1c0-b97bb87affc5@redhat.com> X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: SN4PR0501CA0086.namprd05.prod.outlook.com (2603:10b6:803:22::24) To SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) Return-Path: brijesh.singh@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from Brijeshs-MacBook-Pro.local (70.112.153.56) by SN4PR0501CA0086.namprd05.prod.outlook.com (2603:10b6:803:22::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4219.10 via Frontend Transport; Tue, 8 Jun 2021 14:50:15 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 1a223b44-b116-49ab-2924-08d92a8cbaa4 X-MS-TrafficTypeDiagnostic: SA0PR12MB4509: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8882; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: QcHtmlqNbjTx+mCVeExOzZlogao4pXT/0/tDjPSMK7OQCNfLfid3sCgDzaTc2u4QOSw8dKDfDLA6AZBRePLtm53+iTrNuAYgYhrKvY2uxm0JRvUSpUY2UTGkSQVkV1gGDu6PVPNd2ffAXYft+ZzLHVMHoTOk153SDso5UG8SgdfwXF/QkFKmFwSlOan7oe4+Bi24xaWXYPNErscruS9KHS/OyJcnWRFv1Fw/VdnRYWYKVX8sBO9rkdGFS7B0SRU+KU/w1ghAmJeRnYOdrxAorQYc3KXuhMzvpZEli1Ii43X/5p40QOdj7HcBheHMnA6B/tydt+KA1rBA0buhbIf20dsZoHbRWPIJjmxvlsZpKSa6/IyC1jgHkvJXmDBwcDSC5LWiD3T5ZWzQBH7903xa6vXl6UusBXOMUNjSTojGvmv6Y7V/fYXTn/OyB0T7XfaxtO4hh6KFpk85fqHx4HveFEqUndbrXZbmx1j3gTyqNXf0V+yIemhu9ydA7Cb/nlQ7B+EHu+ft60lIYTM7a2kFRy9VUjoPzdgj7Njvl6CsmD3gRfdqeCGD1JxktvKVyEewdb+cAj6P0gE/pwIO06qxlhKa0ZYsrl2Gy5mjp5gIBOdaxrFo+U3ReX/YSxYrsU6HXnIYbZKzLJl3PPQcdYAn0gv8zEaqifNm3Bl5jtXiFs9n+PhUIw1tZ4Me9lu3dSC/DUWDI7ijMj2zeecb1Qu7gh7Ij/zQTS/impCsN0fOh/U= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SN6PR12MB2718.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(376002)(396003)(346002)(136003)(366004)(39860400002)(31686004)(83380400001)(36756003)(53546011)(6486002)(5660300002)(52116002)(8936002)(31696002)(86362001)(6512007)(186003)(38350700002)(44832011)(38100700002)(2616005)(6916009)(66556008)(66946007)(66476007)(956004)(26005)(54906003)(316002)(7416002)(8676002)(2906002)(16526019)(478600001)(6506007)(4326008)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?WrlifVtsOS3gKz55cc4+TChiPicxHuMv5tZ84PnagImUKNSUZu0ja7+BBdYz?= =?us-ascii?Q?RGqmCjZNU8vhV2L5vlVv90PF8Z+YQyleQV8x9ChnIKEH4Hv55LDCDueIyrnk?= =?us-ascii?Q?kzveM3qtlNUpYHA+wHVNbL+ui09tL48dB5pfjnONuLvgQ/IWIlQ3Q+GpY2RZ?= =?us-ascii?Q?0sY8wc43YV7/Npguy2V/mpiFLWYlB9y6WWuOp5VH8cI0znjgo+2WXdFXhrls?= =?us-ascii?Q?Z3duRW3nwzFcUT0tgLi6+venFfq8kGizwP4k9gnSq/Z/nDAAOzHktaz97ojF?= =?us-ascii?Q?WkPd5Aya2HssgMFa3m5yUY0uNbafCuX0djTOK0BOzNO5KFRKo1yD5lpUkWmt?= =?us-ascii?Q?bJMNAvR5P/jfaJkSbT6/pv1He+impRS1B6+pX+z/jEQjqqcuI/a/qeMx2WbZ?= =?us-ascii?Q?OUMeCo+71Ybi33GIFtD1vS6srIXCQ1Zv7ajeu1PzXgCI6c90cJy0i1QczZU3?= =?us-ascii?Q?oSNoR+m4wiF6r6xj2POvNcORbJOd6ng0mNWQcd5U07YxrezPPTKJ03bIGnAL?= =?us-ascii?Q?TA0y+iWVrWgQABHzySZaDj0naR7g6yT6KWJRA0qyWeaunF9VwEAgP4QUg8fM?= =?us-ascii?Q?ZustAt2Ww0U+0saGpt679WgpmndxoKpOgHoZh1HJOGKsabyXMrRFJKUI6JgG?= =?us-ascii?Q?h9FzUohXgheU3aCJ494lmCeV+u+zDU6rPxD3+Yx/K/QZOxdac08MvzqDJ9QU?= =?us-ascii?Q?YhTT+03WettzXTTIq7TZGKj0zrvH4k6p5i0KdT1OHsayBojMgbamMuM36N/z?= =?us-ascii?Q?BYqZrmsHw47VVxakHE4/1FriJM1L0gSbjggsCT/w6Cl2eyOvo87YxUA7FiVg?= =?us-ascii?Q?zEcqYZthuXgg6V3LMe3ffTUVl9b16PHzhY1NWr0fMVWzIT9LTQXu5NKr2aSR?= =?us-ascii?Q?xkfIWr1uRUE8NA9QzEZbfoO2kNmccaOphcV5Bdu39TpifR1Qcf9G/XKal8I8?= =?us-ascii?Q?bcm53n6OnaDEYRTkHhS+ctszMiIHYiA/hCxyp2u4Mb4m/TSLi/VxKtTnL0S8?= =?us-ascii?Q?qom6glkVeTsWHLLewf0ahEFgjp/7ju4jKYv7Aj+CIWLLBn3D/Qbj986hHnQn?= =?us-ascii?Q?p/MTsuizGUHn8wvayK5Z5Jmcw8f+fSKVP7OqsI+KjX+M2wXPbC1bJSj6jEiP?= =?us-ascii?Q?Pl9IAy9y91He9Oy159ZVunjIZ7Xk75UT1icL13rXPLjq/zEmNGlC1BC/WG2Q?= =?us-ascii?Q?EY/lG8BwntL51tXYx6wnefx465n8Fxof56UnoBguIEhbqv3A2NpN7M7iFGBv?= =?us-ascii?Q?IQHZXgNZpuuRyizOvuirZ3IKsUHd1XFxGQMVQZzeT3CRTOoNe2FrohX13Sv2?= =?us-ascii?Q?jAl2lJQnlu62eRromQMXemgX?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1a223b44-b116-49ab-2924-08d92a8cbaa4 X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jun 2021 14:50:16.5623 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: 9ZrBDh32jh7QZarSZIRHFSlhhSyzMEjYUdxnGI7A06m1yoOyaAtQ+7kfi4t9Mrf3pNzN8VrtSHKgHG3TZXSUcw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4509 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-Language: en-US On 6/8/21 3:49 AM, Laszlo Ersek wrote: > 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.=C2= =A0 >> 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 OVM= F. > > >>> Instead, we should have a new MemEncryptSevLib function that outputs th= e FEATURES bitmask. It should be similar to MemEncryptSevGetEncryptionMask(= ), but it should return a RETURN_STATUS, and produce the FEATURES bitmask t= hrough 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. I have to debug it, but it did appear that this circular dependency caused problem for the SEV guest when SMM is enabled. If SMM is enabled, then I get a random #UD as soon as I link the VmgExit to MemEncryptSevLib. I will see what I find. > > 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 funct= ions. >>> >>> 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. This is much ideal, I would prefer to avoid creating a new library or add a function into the existing library if this function is only going to be used once during the PEI to query the feature. > ... 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 >