* [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases @ 2022-12-15 14:27 Lee, Chun-Yi 2022-12-17 3:17 ` Yao, Jiewen [not found] ` <1731765A81E3FBB0.6436@groups.io> 0 siblings, 2 replies; 6+ messages in thread From: Lee, Chun-Yi @ 2022-12-15 14:27 UTC (permalink / raw) To: devel Cc: Min M Xu, Gerd Hoffmann, Jiewen Yao, Tom Lendacky, James Bottomley, Erdem Aktas, Lee, Chun-Yi 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, 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 saw some variables which 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 changes the falid states list (Follow Jiewen Yao's suggestion): 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. And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because they are invalid states. v2: Follow Jiewen Yao's suggestion to add the following valid states: VAR_ADDED & VAR_DELETED (0x3D) VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED (0x3C) and removed the following invalid states: VAR_IN_DELETED_TRANSITION VAR_DELETED Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> --- OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c b/OvmfPkg/Library/PlatformInitLib/Platform.c index 77f22de046..6963c47e0b 100644 --- a/OvmfPkg/Library/PlatformInitLib/Platform.c +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( VariableOffset = NvVarStoreHeader->Size - sizeof (VARIABLE_STORE_HEADER); } else { - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || - (VariableHeader->State == VAR_DELETED) || - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || - (VariableHeader->State == VAR_ADDED))) + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || + (VariableHeader->State == VAR_ADDED) || + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || + (VariableHeader->State == (VAR_ADDED & VAR_IN_DELETED_TRANSITION)) || + (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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases 2022-12-15 14:27 [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases Lee, Chun-Yi @ 2022-12-17 3:17 ` Yao, Jiewen 2022-12-20 7:09 ` [edk2-devel] " joeyli [not found] ` <1731765A81E3FBB0.6436@groups.io> 1 sibling, 1 reply; 6+ messages in thread From: Yao, Jiewen @ 2022-12-17 3:17 UTC (permalink / raw) To: Lee, Chun-Yi, devel@edk2.groups.io Cc: Xu, Min M, Gerd Hoffmann, Tom Lendacky, James Bottomley, Aktas, Erdem, Lee, Chun-Yi Thanks for the fix. Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> Question: Have you run tiano CI by yourself, before submit the patch? > -----Original Message----- > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > Sent: Thursday, December 15, 2022 10:27 PM > To: devel@edk2.groups.io > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > Subject: [PATCH v2] 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, 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 > saw some variables which 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 changes > the falid states list (Follow Jiewen Yao's suggestion): > > 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. > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because > they are > invalid states. > > v2: > Follow Jiewen Yao's suggestion to add the following valid states: > VAR_ADDED & VAR_DELETED (0x3D) > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > (0x3C) > and removed the following invalid states: > VAR_IN_DELETED_TRANSITION > VAR_DELETED > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > --- > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > b/OvmfPkg/Library/PlatformInitLib/Platform.c > index 77f22de046..6963c47e0b 100644 > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > VariableOffset = NvVarStoreHeader->Size - sizeof > (VARIABLE_STORE_HEADER); > } else { > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > - (VariableHeader->State == VAR_DELETED) || > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > - (VariableHeader->State == VAR_ADDED))) > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > + (VariableHeader->State == VAR_ADDED) || > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > + (VariableHeader->State == (VAR_ADDED & > VAR_IN_DELETED_TRANSITION)) || > + (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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases 2022-12-17 3:17 ` Yao, Jiewen @ 2022-12-20 7:09 ` joeyli 2022-12-21 8:46 ` Yao, Jiewen 0 siblings, 1 reply; 6+ messages in thread From: joeyli @ 2022-12-20 7:09 UTC (permalink / raw) To: devel, jiewen.yao Cc: Lee, Chun-Yi, Xu, Min M, Gerd Hoffmann, Tom Lendacky, James Bottomley, Aktas, Erdem Hi Jiewen, Sorry for I didn't create tiano CI on github in time. And thanks for your help to merge my patch. I will create tiano CI in next time after getting our Review-by tag. Thanks! Joey Lee On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > Thanks for the fix. > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > -----Original Message----- > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > Sent: Thursday, December 15, 2022 10:27 PM > > To: devel@edk2.groups.io > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > Subject: [PATCH v2] 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, 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 > > saw some variables which 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 changes > > the falid states list (Follow Jiewen Yao's suggestion): > > > > 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. > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) because > > they are > > invalid states. > > > > v2: > > Follow Jiewen Yao's suggestion to add the following valid states: > > VAR_ADDED & VAR_DELETED (0x3D) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > (0x3C) > > and removed the following invalid states: > > VAR_IN_DELETED_TRANSITION > > VAR_DELETED > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > --- > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > index 77f22de046..6963c47e0b 100644 > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > (VARIABLE_STORE_HEADER); > > } else { > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > - (VariableHeader->State == VAR_DELETED) || > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > - (VariableHeader->State == VAR_ADDED))) > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > + (VariableHeader->State == VAR_ADDED) || > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > + (VariableHeader->State == (VAR_ADDED & > > VAR_IN_DELETED_TRANSITION)) || > > + (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 > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases 2022-12-20 7:09 ` [edk2-devel] " joeyli @ 2022-12-21 8:46 ` Yao, Jiewen 2022-12-23 5:54 ` joeyli 0 siblings, 1 reply; 6+ messages in thread From: Yao, Jiewen @ 2022-12-21 8:46 UTC (permalink / raw) To: joeyli, devel@edk2.groups.io Cc: Lee, Chun-Yi, Xu, Min M, Gerd Hoffmann, Tom Lendacky, James Bottomley, Aktas, Erdem, Yao, Jiewen Hi Joey You are welcome. Thanks to catch the issue and provide the patch. One thing to clarify: The patch submitter is expected to pass CI, *before* submit the patch. The benefit is that: If the patch passes the review, the maintainer may merge it directly. There is no need to trigger another round CI-fix, V2 patch submission and review. Thank you Yao, Jiewen > -----Original Message----- > From: joeyli <jlee@suse.com> > Sent: Tuesday, December 20, 2022 3:10 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > Cc: Lee, Chun-Yi <joeyli.kernel@gmail.com>; Xu, Min M > <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom > Lendacky <thomas.lendacky@amd.com>; James Bottomley > <jejb@linux.ibm.com>; Aktas, Erdem <erdemaktas@google.com> > Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity > checking failed of NvVarStore in some cases > > Hi Jiewen, > > Sorry for I didn't create tiano CI on github in time. And thanks for your > help to merge my patch. > > I will create tiano CI in next time after getting our Review-by tag. > > Thanks! > Joey Lee > > On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > > Thanks for the fix. > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > -----Original Message----- > > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > > Sent: Thursday, December 15, 2022 10:27 PM > > > To: devel@edk2.groups.io > > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom > Lendacky > > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > > Subject: [PATCH v2] 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, 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 > > > saw some variables which 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 > changes > > > the falid states list (Follow Jiewen Yao's suggestion): > > > > > > 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. > > > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) > because > > > they are > > > invalid states. > > > > > > v2: > > > Follow Jiewen Yao's suggestion to add the following valid states: > > > VAR_ADDED & VAR_DELETED (0x3D) > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > > (0x3C) > > > and removed the following invalid states: > > > VAR_IN_DELETED_TRANSITION > > > VAR_DELETED > > > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > > --- > > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > index 77f22de046..6963c47e0b 100644 > > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > > (VARIABLE_STORE_HEADER); > > > } else { > > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > > - (VariableHeader->State == VAR_DELETED) || > > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > - (VariableHeader->State == VAR_ADDED))) > > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > + (VariableHeader->State == VAR_ADDED) || > > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > > + (VariableHeader->State == (VAR_ADDED & > > > VAR_IN_DELETED_TRANSITION)) || > > > + (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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases 2022-12-21 8:46 ` Yao, Jiewen @ 2022-12-23 5:54 ` joeyli 0 siblings, 0 replies; 6+ messages in thread From: joeyli @ 2022-12-23 5:54 UTC (permalink / raw) To: Yao, Jiewen Cc: devel@edk2.groups.io, Lee, Chun-Yi, Xu, Min M, Gerd Hoffmann, Tom Lendacky, James Bottomley, Aktas, Erdem Hi Jiewen, Thanks for your information. I will follow the process. Joey Lee On Wed, Dec 21, 2022 at 08:46:04AM +0000, Yao, Jiewen wrote: > Hi Joey > You are welcome. Thanks to catch the issue and provide the patch. > > One thing to clarify: The patch submitter is expected to pass CI, *before* submit the patch. > > The benefit is that: If the patch passes the review, the maintainer may merge it directly. > > There is no need to trigger another round CI-fix, V2 patch submission and review. > > Thank you > Yao, Jiewen > > > -----Original Message----- > > From: joeyli <jlee@suse.com> > > Sent: Tuesday, December 20, 2022 3:10 PM > > To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com> > > Cc: Lee, Chun-Yi <joeyli.kernel@gmail.com>; Xu, Min M > > <min.m.xu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Tom > > Lendacky <thomas.lendacky@amd.com>; James Bottomley > > <jejb@linux.ibm.com>; Aktas, Erdem <erdemaktas@google.com> > > Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity > > checking failed of NvVarStore in some cases > > > > Hi Jiewen, > > > > Sorry for I didn't create tiano CI on github in time. And thanks for your > > help to merge my patch. > > > > I will create tiano CI in next time after getting our Review-by tag. > > > > Thanks! > > Joey Lee > > > > On Sat, Dec 17, 2022 at 03:17:42AM +0000, Yao, Jiewen via groups.io wrote: > > > Thanks for the fix. > > > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > > > > > -----Original Message----- > > > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > > > Sent: Thursday, December 15, 2022 10:27 PM > > > > To: devel@edk2.groups.io > > > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom > > Lendacky > > > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > > > Subject: [PATCH v2] 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, 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 > > > > saw some variables which 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 > > changes > > > > the falid states list (Follow Jiewen Yao's suggestion): > > > > > > > > 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. > > > > > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) > > because > > > > they are > > > > invalid states. > > > > > > > > v2: > > > > Follow Jiewen Yao's suggestion to add the following valid states: > > > > VAR_ADDED & VAR_DELETED (0x3D) > > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > > > (0x3C) > > > > and removed the following invalid states: > > > > VAR_IN_DELETED_TRANSITION > > > > VAR_DELETED > > > > > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > > > --- > > > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > index 77f22de046..6963c47e0b 100644 > > > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > > > (VARIABLE_STORE_HEADER); > > > > } else { > > > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > > > - (VariableHeader->State == VAR_DELETED) || > > > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > > - (VariableHeader->State == VAR_ADDED))) > > > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > > > + (VariableHeader->State == VAR_ADDED) || > > > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > > > + (VariableHeader->State == (VAR_ADDED & > > > > VAR_IN_DELETED_TRANSITION)) || > > > > + (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 > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1731765A81E3FBB0.6436@groups.io>]
* Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases [not found] ` <1731765A81E3FBB0.6436@groups.io> @ 2022-12-20 5:34 ` Yao, Jiewen 0 siblings, 0 replies; 6+ messages in thread From: Yao, Jiewen @ 2022-12-20 5:34 UTC (permalink / raw) To: devel@edk2.groups.io, Yao, Jiewen, Lee, Chun-Yi Cc: Xu, Min M, Gerd Hoffmann, Tom Lendacky, James Bottomley, Aktas, Erdem, Lee, Chun-Yi Merged https://github.com/tianocore/edk2/pull/3797 with minor format fix. > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, > Jiewen > Sent: Saturday, December 17, 2022 11:18 AM > To: Lee, Chun-Yi <joeyli.kernel@gmail.com>; devel@edk2.groups.io > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > <kraxel@redhat.com>; Tom Lendacky <thomas.lendacky@amd.com>; James > Bottomley <jejb@linux.ibm.com>; Aktas, Erdem <erdemaktas@google.com>; > Lee, Chun-Yi <jlee@suse.com> > Subject: Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity > checking failed of NvVarStore in some cases > > Thanks for the fix. > Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > -----Original Message----- > > From: Lee, Chun-Yi <joeyli.kernel@gmail.com> > > Sent: Thursday, December 15, 2022 10:27 PM > > To: devel@edk2.groups.io > > Cc: Xu, Min M <min.m.xu@intel.com>; Gerd Hoffmann > > <kraxel@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom > Lendacky > > <thomas.lendacky@amd.com>; James Bottomley <jejb@linux.ibm.com>; > > Aktas, Erdem <erdemaktas@google.com>; Lee, Chun-Yi <jlee@suse.com> > > Subject: [PATCH v2] 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, 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 > > saw some variables which 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 changes > > the falid states list (Follow Jiewen Yao's suggestion): > > > > 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. > > > > And removed (VAR_IN_DELETED_TRANSITION) and (VAR_DELETED) > because > > they are > > invalid states. > > > > v2: > > Follow Jiewen Yao's suggestion to add the following valid states: > > VAR_ADDED & VAR_DELETED (0x3D) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION (0x3E) > > VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED > > (0x3C) > > and removed the following invalid states: > > VAR_IN_DELETED_TRANSITION > > VAR_DELETED > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com> > > --- > > OvmfPkg/Library/PlatformInitLib/Platform.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c > > b/OvmfPkg/Library/PlatformInitLib/Platform.c > > index 77f22de046..6963c47e0b 100644 > > --- a/OvmfPkg/Library/PlatformInitLib/Platform.c > > +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c > > @@ -702,10 +702,11 @@ PlatformValidateNvVarStore ( > > > > VariableOffset = NvVarStoreHeader->Size - sizeof > > (VARIABLE_STORE_HEADER); > > } else { > > - if (!((VariableHeader->State == VAR_IN_DELETED_TRANSITION) || > > - (VariableHeader->State == VAR_DELETED) || > > - (VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > - (VariableHeader->State == VAR_ADDED))) > > + if (!((VariableHeader->State == VAR_HEADER_VALID_ONLY) || > > + (VariableHeader->State == VAR_ADDED) || > > + (VariableHeader->State == (VAR_ADDED & VAR_DELETED)) || > > + (VariableHeader->State == (VAR_ADDED & > > VAR_IN_DELETED_TRANSITION)) || > > + (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 > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-23 5:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-15 14:27 [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases Lee, Chun-Yi 2022-12-17 3:17 ` Yao, Jiewen 2022-12-20 7:09 ` [edk2-devel] " joeyli 2022-12-21 8:46 ` Yao, Jiewen 2022-12-23 5:54 ` joeyli [not found] ` <1731765A81E3FBB0.6436@groups.io> 2022-12-20 5:34 ` Yao, Jiewen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox