From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from EUR04-DB3-obe.outbound.protection.outlook.com (EUR04-DB3-obe.outbound.protection.outlook.com [40.107.6.51]) by mx.groups.io with SMTP id smtpd.web11.106559.1671030343172709880 for ; Wed, 14 Dec 2022 07:05:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@suse.com header.s=selector1 header.b=ORg+ZjCb; spf=pass (domain: suse.com, ip: 40.107.6.51, mailfrom: jlee@suse.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Dz55rZjbt3asLgxUiVVsi7YxMmTx0E346k0Cx8JRf4RQqjuev65mrgYjHqeHiaB3iI66fJ9XKb/TZMZ4fhJyWaNGNks0Z4T1KOtmI8fEK9ZLOqR0nYbZGM8UnS2wF01SLW4xnmPV6rAM47R2TLIxBSeq9lAojzUwxRClL25hz/hAs58NoTR07qiv9JdUPrsbC/o1g9y8LHdJ1IXuRqduAgWDc2tH5syKo5YYeymgWCPxVFtETa0pm7eT+0KPWdfg3eaH16fijIdl+LU4OhZwj9b2YzlWdj9glEPMmyGBUdrbC8FVYkMqTuWe/MKQ9/VpUd/SEvU6HdDq2ZYzsU3gMg== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Z3ylaDETwIc0fYXheARjFk1ASaIaCCg2v70AVQ0gGa0=; b=a4zuMmRL0UKbS+1BURaOXPJyNaDTnwB34ZblEDphpKJtaYEF94m7Z5zLzGvflJo7LF2rWi0xb4L4PW+BhOt9/GkxktBvD4zK+qKUzkA+KT70trxJ4ba7swLZ2Z3mjfn1KM/WcC8xxqtWEZmExOaxuzHyqbu6EjlAouvYns2Dlta9iTm0wT+oZb3TnRVWWriSI4t50ef7XxdVxknxAaFyRu9DYIkxr8fLXa85X3wiEZaKr/mSRbilF2bDK+7rNgNJVGiyFTC37lboTBYEhFU5/l/K6eBkS2inMDtJr4nogCQehJUOk6DmTrn7ZFBLhZZfHJDWzdPduvVuT3I0u8vIkw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Z3ylaDETwIc0fYXheARjFk1ASaIaCCg2v70AVQ0gGa0=; b=ORg+ZjCbbobVAtppDOt2V3dCR2A2W7vf2ryWNRPpgT8H55+48xqPhrJjgdMwD2JVyaQQ8qJtNdnyknKUqS6b0Bw2UhDnZmYc8sXzuLJxWlJe4VnaLqmfgULtQOpiMsCeSXslqfRkhQ2s4tP9SsBZRmMgnA2U8l1kaTEYoqhwq/QLC14sBm7FHQBg6Izz4CT7+NPwS9qm/YSLOHvpA8hvONgPHDWf7guDIYf8keA9qRbw+C5PG2N++2b/DuCFuebab3puu1HmflnrocSVkUcjBFfnk/rTIc+AAmHyWRCJPZI2aQcXmI8sL6zNzTGVfxKnTnt+WpX3B6eGddJiPQj4Qg== Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com; Received: from DB8PR04MB7164.eurprd04.prod.outlook.com (2603:10a6:10:129::23) by DB9PR04MB8479.eurprd04.prod.outlook.com (2603:10a6:10:2c5::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5880.11; Wed, 14 Dec 2022 15:05:40 +0000 Received: from DB8PR04MB7164.eurprd04.prod.outlook.com ([fe80::3cc9:78f1:6e76:b1ef]) by DB8PR04MB7164.eurprd04.prod.outlook.com ([fe80::3cc9:78f1:6e76:b1ef%5]) with mapi id 15.20.5880.019; Wed, 14 Dec 2022 15:05:40 +0000 Date: Wed, 14 Dec 2022 23:05:28 +0800 From: "joeyli" To: devel@edk2.groups.io, jiewen.yao@intel.com Cc: "Lee, Chun-Yi" , "Xu, Min M" , Gerd Hoffmann , Tom Lendacky , James Bottomley , "Aktas, Erdem" Subject: Re: [edk2-devel] [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases Message-ID: <20221214150528.GA11807@linux-l9pv.suse> References: <20221213155502.29548-1-jlee@suse.com> In-Reply-To: User-Agent: Mutt/1.11.4 (2019-03-13) X-ClientProxiedBy: TYCP301CA0012.JPNP301.PROD.OUTLOOK.COM (2603:1096:400:386::10) To DB8PR04MB7164.eurprd04.prod.outlook.com (2603:10a6:10:129::23) Return-Path: JLee@suse.com MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: DB8PR04MB7164:EE_|DB9PR04MB8479:EE_ X-MS-Office365-Filtering-Correlation-Id: d5f66030-6787-4b43-600d-08dadde4aa29 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 9OvQSQb0zRXVPn0l/UNPMmKobVRPSEYpbeEm1dGwju4J23JMNWMKKzyMaDbF7nLtRcv+BF35NS/fBV6vTJfMIVJahussjMdkbrlU88SrqjBN6o/1zCS2KDDveJS+dLIPPVhOckOAWRzC6mBLNAPvsJuy37VixtDTnUqLmf3wIjSUWQLJ6SW9Y15ds/WIZC6opeuh72GH0N9Pcloh3N6eQNk3to4TnbB0PaSXBFFh24JsMYNaciIbOR4O6WyIpBhFPHwOcN3WxJqp40V71lWr2rb0Ybc7rp+301LmmNkj3zYL5dsj6+TQC+cFXhg6Arb+P3Z2gH5InKIZignx1cc1z7U7B43tV6S6QDJsVWFV6nPV9tielQ2MWwBI/pj6Qxs4yQJPXcCa1EhS4QtAjE8ExGNBlmrcfKCqO5Fyd8C6O1nxLvshGn1KehmDgGhUVfEnRnQsBjb5Mtm1sVMNgHuciOYYYnDqYajH/ruz2Gg1aamYB6KuPv2Ho8Ffd9TAix7ZJmFDEPdytnQz+IzmDek0BPXZHi+qxiYiJWMQYVJ8aBw+heieZSZk2h/lUNVoL3JBFwIWhwdmSsiV23F2GXJWLcXmdcwPQjdk6NpRzV8ergJ3XU4GFaVcbRCxmFZXBGYik2EL5ZfWynzlaHWg2K04uX+ti3kmtOYQiObeasnPUwH1bLVrJa1pH6/P+RpT9OO2jgxD8uNe6Tb03O2YLcHNcw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DB8PR04MB7164.eurprd04.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230022)(6029001)(396003)(136003)(39860400002)(346002)(376002)(366004)(451199015)(83380400001)(1076003)(8676002)(86362001)(4326008)(33656002)(36756003)(8936002)(41300700001)(5660300002)(54906003)(26005)(316002)(966005)(6486002)(19627235002)(478600001)(38100700002)(66556008)(66946007)(66476007)(186003)(9686003)(6506007)(6512007)(6666004)(53546011)(66899015)(84970400001)(2906002)(43062005);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3rlWFJGSmuamZVEKAPPnx6hA24bti9dx1jMhB2AM2U+OTBec/TUlC3Zxjukk?= =?us-ascii?Q?u9RSWzvpuQ2LMIyktZhhvwI1Fw5pb7T/JOxAqjnJ/uWZKmV+3fWXLog7E0sY?= =?us-ascii?Q?U7zh9m30OVaeRlUUfP5Wd2TGODp2Uo36l2FvH8yN+V64sb56nAM0UjSf9XXm?= =?us-ascii?Q?0KBi+4M7FEDsQDU0zGe27DW5/2/dme8t0BzPExaNhAc5lbQVjI8+ziWM4pFF?= =?us-ascii?Q?KaOAPR1ReExMj1CLeDFraXNYlHmBzFJxqUXLB5mavoEc9BV1D3uoxFkhNe2m?= =?us-ascii?Q?Yz3ILqa42yN9Ee0ZV6QQoDj/ciVnBeiP19t7UUA6yMOxHePz/4Wz/EznZIqe?= =?us-ascii?Q?AKGfTZmpASMPzWeS0f8UtwlQcMIjCoHpXa0UWEdFQ+1BJzXDDFoL6vIEXEcg?= =?us-ascii?Q?o93dTi2hj6L4g1YyNnzhFfP/suQ3mFrjR2UBuKsiFbcmdvUG73xYQDRBUPSS?= =?us-ascii?Q?KshCwHOgWPKvyp+PYwlewnbWteB3qP2fDVsfUqsKCQgH05tfDBRiNxGHMtm0?= =?us-ascii?Q?Qz3r/zGG1+Tivt10FMhy+BImmk8WaojPt4ICcAreDL5ys3gNSJuD2Kh1chBs?= =?us-ascii?Q?HgOQgX1+1Ar2IiF0ZZUnT2vWMUjcNiUsRKVOQ+HywsgeKsplTTLPxHwZIWlQ?= =?us-ascii?Q?8ABDmE60pHH/MMDUaxCYLB19oOjse1DVwQpX3LhLu1PP6ZsDs4EeAp+zeGr0?= =?us-ascii?Q?vwu0ykjpI4INoDiYRz+BY7CaNWqE6LPnmdlJo3SVAG/jHx8WgjXnH4ISQDSk?= =?us-ascii?Q?oc8kVySwsaNtAjO4gMIFENNRiS478h04jZZKd4YR1YCHMB6OX3VMWBqgRluH?= =?us-ascii?Q?0Th0fogAAMOdjnzBW+8sKyv4lIdFBhtxteg10fHhPkkaD8Yyhti94TlNNj7b?= =?us-ascii?Q?EFDlvImCLkwmXjGoVcsJdQLiKGA5t2lvbRDTVK9YCiYpZgP9Uzu7EOfBuhEe?= =?us-ascii?Q?9nZy6LKux/Sw2x8Z5bxp64C3MMr2ci22ScL7kfLWcwagREr1AydJ8LI5SrsS?= =?us-ascii?Q?MxbJlouRd5iF7wK2XqlVIZ8gpHwHrhziminB9AlUOqBqEwmBK5prUkEyEbGU?= =?us-ascii?Q?pGTvXqPXy3WWLPqxK5yj0u352/c9THYtDYq8CsStlMEux9wiuGBaTVj5GtlQ?= =?us-ascii?Q?JVjjGfDl06KYWkVfuOEdEX1pSG0LXnZnHHsI5KsTEOdnRUcADtwd6XlEAgOc?= =?us-ascii?Q?fnYIodI3ZCtEJcQ4Lrrc1hDoCOAPzqYcF5lVQ78O5PfGWvBECxyQ4tFaT2lf?= =?us-ascii?Q?zyNCCSO7BzwoqoGxsefu/+uyV3u/chCiEbZVIh5xOFb50aaHTUdgO5bEOdR+?= =?us-ascii?Q?xeAV/fE34zk6fT9uhSa3rcIERe+jhUot2mV4YhfufNtdiarGkdQcNfyxUerD?= =?us-ascii?Q?cgn3EqGppmm4m/IaLvTzhDm0QcstfwFTQUrdknOSyE6Xp0a3uHihppzHecKn?= =?us-ascii?Q?hsprFRMVhmCbMQOm6TiEKmB+mYAX6KKas/vrtFLCFA0COaEIwiMEDQPsR8m4?= =?us-ascii?Q?XDvlyuj3wLP8iROSPp/yEwDIe9fj8t0j2awNml61pm0/1O3sueLJprfImveZ?= =?us-ascii?Q?/EyGJn+RTrEGdZZDz00=3D?= X-OriginatorOrg: suse.com X-MS-Exchange-CrossTenant-Network-Message-Id: d5f66030-6787-4b43-600d-08dadde4aa29 X-MS-Exchange-CrossTenant-AuthSource: DB8PR04MB7164.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2022 15:05:40.5029 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: Fd3NndxXOsxkIjMrgTn6+b1NIwE8L2KQ8XGCO+6VpeAyAii771Uk7CRiSIO7VSHA X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR04MB8479 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Jiewen, Thanks for your response! On Wed, Dec 14, 2022 at 06:53:42AM +0000, Yao, Jiewen via groups.io wrote: > Hey > Good catch! > > I think we need handle below valid cases: > 1. VAR_HEADER_VALID_ONLY (0x7F) <-- Header added (*) > 2. VAR_ADDED (0x3F) <-- Header + data added > 3. VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) <-- marked as deleted, but still valid, before new data is added. (*) > 4. VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) <-- deleted, after new data is added. > 5. VAR_ADDED & VAR_DELETED (0x3D) <-- deleted directly, without new data. > (*) means to support surprise shutdown. > > > For the patch: > > if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > (VariableHeader->State == VAR_DELETED) || > > (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > - (VariableHeader->State == VAR_ADDED))) > > + (VariableHeader->State == VAR_ADDED) || > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > + (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > > I think: > A. If we allow (VAR_HEADER_VALID_ONLY), then we support surprise shutdown, we need also allow (VAR_ADDED & VAR_IN_DELETED_TRANSITION). It should be added as well. > B. The (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) are invalid state. They should be removed. > > Would you please double check? > I have followed your suggestion to change patch to add (VAR_ADDED & VAR_IN_DELETED_TRANSITION) and removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED). Like this: Index: edk2/OvmfPkg/Library/PlatformInitLib/Platform.c =================================================================== --- edk2.orig/OvmfPkg/Library/PlatformInitLib/Platform.c +++ edk2/OvmfPkg/Library/PlatformInitLib/Platform.c @@ -704,10 +704,11 @@ PlatformValidateNvVarStore ( VariableOffset = NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER); } else { DEBUG ((DEBUG_ERROR, "VariableHeader->VendorGuid = %g, VariableHeader->State = 0x%x\n", VariableHeader->VendorGuid, VariableHeader->State)); - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) ||^M - (VariableHeader->State == VAR_DELETED) ||^M - (VariableHeader->State == VAR_HEADER_VALID_ONLY) ||^M - (VariableHeader->State == VAR_ADDED)))^M + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) ||^M + (VariableHeader->State == VAR_ADDED) ||^M + (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION)) ||^M + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) ||^M + (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED))))^M { DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was invalid.\n")); return FALSE; The above change works to me no my OVMF. Thanks a lot! Joey Lee > > > > > -----Original Message----- > > From: Lee, Chun-Yi > > Sent: Tuesday, December 13, 2022 11:55 PM > > To: devel@edk2.groups.io > > Cc: Xu, Min M ; Gerd Hoffmann > > ; Yao, Jiewen ; Tom Lendacky > > ; James Bottomley ; > > Aktas, Erdem ; Lee, Chun-Yi > > Subject: [PATCH] OvmfPkg/PlatformInitLib: Fix integrity checking failed of > > NvVarStore in some cases > > > > In the commit 4f173db8b4 "OvmfPkg/PlatformInitLib: Add functions for > > EmuVariableNvStore" > > , it introduced a PlatformValidateNvVarStore() function for checking the > > integrity of NvVarStore. > > > > In some cases when the VariableHeader->StartId is VARIABLE_DATA, the > > VariableHeader->State > > is not just one of the four primary states: VAR_IN_DELETED_TRANSITION, > > VAR_DELETED, > > VAR_HEADER_VALID_ONLY, VAR_ADDED. The state may combined two or > > three > > states, e.g. > > 0x3C = (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & VAR_DELETED > > or > > 0x3D = VAR_ADDED & VAR_DELETED > > > > When the variable store has those variables, then system booting/rebooting > > will > > hangs in a ASSERT: > > > > NvVarStore Variable header State was invalid. > > ASSERT > > /mnt/working/source_code- > > git/edk2/OvmfPkg/Library/PlatformInitLib/Platform.c(819): > > ((BOOLEAN)(0==1)) > > > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we > > can see there have some variables have 0x3C or 0x3D state in store. > > e.g. > > > > UpdateVariable(), VariableName=BootOrder > > L1871, State=0000003F <-- VAR_ADDED > > State &= VAR_DELETED=0000003D > > FlushHobVariableToFlash(), VariableName=BootOrder > > ... > > UpdateVariable(), VariableName=InitialAttemptOrder > > L1977, State=0000003F > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > L2376, State=0000003E > > State &= VAR_DELETED=0000003C > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > ... > > UpdateVariable(), VariableName=ConIn > > L1977, State=0000003F > > State &= VAR_IN_DELETED_TRANSITION=0000003E > > L2376, State=0000003E > > State &= VAR_DELETED=0000003C > > FlushHobVariableToFlash(), VariableName=ConIn > > ... > > > > So, only allowing the four primary states is not enough. This patch adds > > two more combined states to the valid states list: > > > > (VAR_IN_DELETED_TRANSITION & VAR_ADDED) & VAR_DELETED = 0x3c > > > > VAR_ADDED & VAR_DELETED = 0x3d > > > > Signed-off-by: "Lee, Chun-Yi" > > --- > > OvmfPkg/Library/PlatformInitLib/Platform.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > index 77f22de046..2af4cefd10 100644 > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > @@ -705,7 +705,9 @@ PlatformValidateNvVarStore ( > > if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > (VariableHeader->State == VAR_DELETED) || > > (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > - (VariableHeader->State == VAR_ADDED))) > > + (VariableHeader->State == VAR_ADDED) || > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > + (VariableHeader->State == (VAR_ADDED & > > VAR_IN_DELETED_TRANSITION & VAR_DELETED)))) > > { > > DEBUG ((DEBUG_ERROR, "NvVarStore Variable header State was > > invalid.\n")); > > return FALSE; > > -- > > 2.35.3 > > > > >