Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Hi Jiewen, Thanks for your information. I will follow the process. Joey Lee On Wed, Dec 21, 2022 at 08:46:04AM +, 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 > > Sent: Tuesday, December 20, 2022 3:10 PM > > To: devel@edk2.groups.io; Yao, Jiewen > > Cc: Lee, Chun-Yi ; Xu, Min M > > ; Gerd Hoffmann ; Tom > > Lendacky ; James Bottomley > > ; Aktas, Erdem > > 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 +, Yao, Jiewen via groups.io wrote: > > > Thanks for the fix. > > > Reviewed-by: Jiewen Yao > > > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > > > > > -Original Message- > > > > From: Lee, Chun-Yi > > > > Sent: Thursday, December 15, 2022 10:27 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 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=003F <-- VAR_ADDED > > > > State &= VAR_DELETED=003D > > > > FlushHobVariableToFlash(), VariableName=BootOrder > > > > ... > > > > UpdateVariable(), VariableName=InitialAttemptOrder > > > > L1977, State=003F > > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > > L2376, State=003E > > > > State &= VAR_DELETED=003C > > > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > > > ... > > > > UpdateVariable(), VariableName=ConIn > > > > L1977, State=003F > > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > > L2376, State=003E > > > > State &= VAR_DELETED=003C > > > > FlushHobVariableToFlash(), VariableName=ConIn > > > > ... > > > > > > > > So, only allowing the four primary states is not enough. This patch > > changes > > > > the falid states list (Follow Jiewen Yao's suggestio
Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
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 > Sent: Tuesday, December 20, 2022 3:10 PM > To: devel@edk2.groups.io; Yao, Jiewen > Cc: Lee, Chun-Yi ; Xu, Min M > ; Gerd Hoffmann ; Tom > Lendacky ; James Bottomley > ; Aktas, Erdem > 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 +, Yao, Jiewen via groups.io wrote: > > Thanks for the fix. > > Reviewed-by: Jiewen Yao > > > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > > > > > -Original Message- > > > From: Lee, Chun-Yi > > > Sent: Thursday, December 15, 2022 10:27 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 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=003F <-- VAR_ADDED > > > State &= VAR_DELETED=003D > > > FlushHobVariableToFlash(), VariableName=BootOrder > > > ... > > > UpdateVariable(), VariableName=InitialAttemptOrder > > > L1977, State=003F > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > L2376, State=003E > > > State &= VAR_DELETED=003C > > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > > ... > > > UpdateVariable(), VariableName=ConIn > > > L1977, State=003F > > > State &= VAR_IN_DELETED_TRANSITION=003E > > > L2376, State=003E > > > State &= VAR_DELETED=003C > > > 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) >
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 +, Yao, Jiewen via groups.io wrote:
> Thanks for the fix.
> Reviewed-by: Jiewen Yao
>
> Question: Have you run tiano CI by yourself, before submit the patch?
>
>
>
> > -Original Message-
> > From: Lee, Chun-Yi
> > Sent: Thursday, December 15, 2022 10:27 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 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=003F <-- VAR_ADDED
> > State &= VAR_DELETED=003D
> > FlushHobVariableToFlash(), VariableName=BootOrder
> > ...
> > UpdateVariable(), VariableName=InitialAttemptOrder
> > L1977, State=003F
> > State &= VAR_IN_DELETED_TRANSITION=003E
> > L2376, State=003E
> > State &= VAR_DELETED=003C
> > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
> > ...
> > UpdateVariable(), VariableName=ConIn
> > L1977, State=003F
> > State &= VAR_IN_DELETED_TRANSITION=003E
> > L2376, State=003E
> > State &= VAR_DELETED=003C
> > 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"
> > ---
> > 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
>
>
>
>
>
Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Merged https://github.com/tianocore/edk2/pull/3797 with minor format fix. > -Original Message- > From: devel@edk2.groups.io On Behalf Of Yao, > Jiewen > Sent: Saturday, December 17, 2022 11:18 AM > To: Lee, Chun-Yi ; devel@edk2.groups.io > Cc: Xu, Min M ; Gerd Hoffmann > ; Tom Lendacky ; James > Bottomley ; Aktas, Erdem ; > Lee, Chun-Yi > 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 > > Question: Have you run tiano CI by yourself, before submit the patch? > > > > > -Original Message- > > From: Lee, Chun-Yi > > Sent: Thursday, December 15, 2022 10:27 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 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=003F <-- VAR_ADDED > > State &= VAR_DELETED=003D > > FlushHobVariableToFlash(), VariableName=BootOrder > > ... > > UpdateVariable(), VariableName=InitialAttemptOrder > > L1977, State=003F > > State &= VAR_IN_DELETED_TRANSITION=003E > > L2376, State=003E > > State &= VAR_DELETED=003C > > FlushHobVariableToFlash(), VariableName=InitialAttemptOrder > > ... > > UpdateVariable(), VariableName=ConIn > > L1977, State=003F > > State &= VAR_IN_DELETED_TRANSITION=003E > > L2376, State=003E > > State &= VAR_DELETED=003C > > 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" > > --- > > 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/Platfor
[edk2-devel] [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=003F <-- VAR_ADDED
State &= VAR_DELETED=003D
FlushHobVariableToFlash(), VariableName=BootOrder
...
UpdateVariable(), VariableName=InitialAttemptOrder
L1977, State=003F
State &= VAR_IN_DELETED_TRANSITION=003E
L2376, State=003E
State &= VAR_DELETED=003C
FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
...
UpdateVariable(), VariableName=ConIn
L1977, State=003F
State &= VAR_IN_DELETED_TRANSITION=003E
L2376, State=003E
State &= VAR_DELETED=003C
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"
---
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97581): https://edk2.groups.io/g/devel/message/97581
Mute This Topic: https://groups.io/mt/95777531/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2] OvmfPkg/PlatformInitLib: Fix integrity checking failed of NvVarStore in some cases
Thanks for the fix.
Reviewed-by: Jiewen Yao
Question: Have you run tiano CI by yourself, before submit the patch?
> -Original Message-
> From: Lee, Chun-Yi
> Sent: Thursday, December 15, 2022 10:27 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 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=003F <-- VAR_ADDED
> State &= VAR_DELETED=003D
> FlushHobVariableToFlash(), VariableName=BootOrder
> ...
> UpdateVariable(), VariableName=InitialAttemptOrder
> L1977, State=003F
> State &= VAR_IN_DELETED_TRANSITION=003E
> L2376, State=003E
> State &= VAR_DELETED=003C
> FlushHobVariableToFlash(), VariableName=InitialAttemptOrder
> ...
> UpdateVariable(), VariableName=ConIn
> L1977, State=003F
> State &= VAR_IN_DELETED_TRANSITION=003E
> L2376, State=003E
> State &= VAR_DELETED=003C
> 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"
> ---
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97523): https://edk2.groups.io/g/devel/message/97523
Mute This Topic: https://groups.io/mt/95724215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
