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" <jlee@suse.com>
---
 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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97329): https://edk2.groups.io/g/devel/message/97329
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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?




> -----Original Message-----
> From: Lee, Chun-Yi <joeyli.kernel@gmail.com>
> Sent: Tuesday, December 13, 2022 11:55 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] 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" <jlee@suse.com>
> ---
>  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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97345): https://edk2.groups.io/g/devel/message/97345
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 <joeyli.kernel@gmail.com>
> > Sent: Tuesday, December 13, 2022 11:55 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] 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" <jlee@suse.com>
> > ---
> >  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
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97371): https://edk2.groups.io/g/devel/message/97371
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> 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))

I'm wondering how you manage to trigger this assert?

> Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we
> can see there have some variables have 0x3C or 0x3D state in store.
> e.g.

PlatformValidateNvVarStore() validates the varstore in ROM before
copying it to RAM.  The normal UpdateVariable() cycle changes the
copy in RAM ...

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97344): https://edk2.groups.io/g/devel/message/97344
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Gerd,

Thanks for your response! 

On Wed, Dec 14, 2022 at 07:15:28AM +0100, Gerd Hoffmann via groups.io wrote:
>   Hi,
> 
> > 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))
> 
> I'm wondering how you manage to trigger this assert?
>

Sorry for I forgot to put my testing environment in patch description.
My testing is on qemu with OVMF:

- edk2-master or edk2-stable202211
	build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D TPM_CONFIG_ENABLE \
	-D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t GCC5 \
	-p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE 

- qemu-7.1.0 with libvirt-8.0.0
  pc-q35 with pflash type and nvram:
    <type arch='x86_64' machine='pc-q35-3.1'>hvm</type>
    <loader readonly='yes' secure='no' type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin</loader>
    <nvram template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd</nvram>
 
- grub2 2.06 and Linux kernel 6.1.0-rc5

Two test cases, both of them can reproduce the assert:

- First booting to grub2 menu, then "virsh destroy" the guest.
  The second boot hangs in the "NvVarStore Variable header State was invalid." assert

- First boot to Linux kernel 6.1.0-rc5, bash shell prompt shows up.
  Then run shutdown or reboot command.
  The second boot hangs in in the "NvVarStore Variable header State was invalid." assert 
 
> > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we
> > can see there have some variables have 0x3C or 0x3D state in store.
> > e.g.
> 
> PlatformValidateNvVarStore() validates the varstore in ROM before
> copying it to RAM.  The normal UpdateVariable() cycle changes the
> copy in RAM ...
>

Yes, in the first boot, those variables have 0x3C or 0x3C state be
created. After shutdown or reboot, system hangs in the second boot.
The assert shows up in debug log.

Thanks a lot!
Joey Lee


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97367): https://edk2.groups.io/g/devel/message/97367
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi,

On Wed, Dec 14, 2022 at 09:46:36PM +0800, joeyli wrote:
> Hi Gerd,
> 
> Thanks for your response! 
> 
> On Wed, Dec 14, 2022 at 07:15:28AM +0100, Gerd Hoffmann via groups.io wrote:
> >   Hi,
> > 
> > > 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))
> > 
> > I'm wondering how you manage to trigger this assert?
> >
> 
> Sorry for I forgot to put my testing environment in patch description.
> My testing is on qemu with OVMF:
> 
> - edk2-master or edk2-stable202211
> 	build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D TPM_CONFIG_ENABLE \
> 	-D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t GCC5 \
> 	-p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE 
> 
> - qemu-7.1.0 with libvirt-8.0.0
>   pc-q35 with pflash type and nvram:
>     <type arch='x86_64' machine='pc-q35-3.1'>hvm</type>
>     <loader readonly='yes' secure='no' type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin</loader>
>     <nvram template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd</nvram>
>  
> - grub2 2.06 and Linux kernel 6.1.0-rc5
>

I forgot shim-15.7, the fallback mechanism created new boot entry and
update bootorder in a new *_VARS.fd :

FSOpen: Open '\EFI\opensuse\boot.csv' Success
UpdateVariable() - Boot0003, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C
FlushHobVariableToFlash() - Boot0003, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C
UpdateVariable() - BootOrder, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C 
L1870, State = 0x3F
State &= VAR_DELETED = 0x3D		<-- state of BootOrder
FlushHobVariableToFlash() - BootOrder, 8BE4DF61-93CA-11D2-AA0D-00E098032B8C

Joey Lee
 
> Two test cases, both of them can reproduce the assert:
> 
> - First booting to grub2 menu, then "virsh destroy" the guest.
>   The second boot hangs in the "NvVarStore Variable header State was invalid." assert
> 
> - First boot to Linux kernel 6.1.0-rc5, bash shell prompt shows up.
>   Then run shutdown or reboot command.
>   The second boot hangs in in the "NvVarStore Variable header State was invalid." assert 
>  
> > > Adding more log to UpdateVariable() and PlatformValidateNvVarStore(), we
> > > can see there have some variables have 0x3C or 0x3D state in store.
> > > e.g.
> > 
> > PlatformValidateNvVarStore() validates the varstore in ROM before
> > copying it to RAM.  The normal UpdateVariable() cycle changes the
> > copy in RAM ...
> >
> 
> Yes, in the first boot, those variables have 0x3C or 0x3C state be
> created. After shutdown or reboot, system hangs in the second boot.
> The assert shows up in debug log.
> 
> Thanks a lot!
> Joey Lee


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97370): https://edk2.groups.io/g/devel/message/97370
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

> Sorry for I forgot to put my testing environment in patch description.
> My testing is on qemu with OVMF:
> 
> - edk2-master or edk2-stable202211
> 	build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D TPM_CONFIG_ENABLE \
> 	-D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t GCC5 \
> 	-p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE 
> 
> - qemu-7.1.0 with libvirt-8.0.0
>   pc-q35 with pflash type and nvram:
>     <type arch='x86_64' machine='pc-q35-3.1'>hvm</type>
>     <loader readonly='yes' secure='no' type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin</loader>
>     <nvram template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd</nvram>

That is not secure.  You have unprotected writable flash.

You can either use a build with SMM_REQUIRE=TRUE and run with
secure='yes', so only the firmware in SMM mode can write to flash.

Or you run with both code and vars read-only.
Easiest is <loader>OVMF.fd</loader>.

Or you disable secure boot (SECURE_BOOT_ENABLE=FALSE) in your
builds.  You still have unprotected writable flash then, but
it isn't a security hole any more.  And the assert isn't triggered
either because that code path is only executed for secure boot
builds.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97368): https://edk2.groups.io/g/devel/message/97368
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Dec 14, 2022 at 03:12:22PM +0100, Gerd Hoffmann wrote:
> > Sorry for I forgot to put my testing environment in patch description.
> > My testing is on qemu with OVMF:
> > 
> > - edk2-master or edk2-stable202211
> > 	build --verbose --debug=1 -D SECURE_BOOT_ENABLE -D TPM_ENABLE -D TPM_CONFIG_ENABLE \
> > 	-D NETWORK_IP6_ENABLE -D NETWORK_HTTP_BOOT_ENABLE -a X64 -b DEBUG -t GCC5 \
> > 	-p OvmfPkg/OvmfPkgX64.dsc -D FD_SIZE_4MB -D NETWORK_TLS_ENABLE 
> > 
> > - qemu-7.1.0 with libvirt-8.0.0
> >   pc-q35 with pflash type and nvram:
> >     <type arch='x86_64' machine='pc-q35-3.1'>hvm</type>
> >     <loader readonly='yes' secure='no' type='pflash'>/usr/share/qemu/ovmf-x86_64-code.bin</loader>
> >     <nvram template='/usr/share/qemu/ovmf-x86_64-vars.bin'>/var/lib/libvirt/qemu/nvram/opensuseTW_VARS.fd</nvram>
> 
> That is not secure.  You have unprotected writable flash.
> 
> You can either use a build with SMM_REQUIRE=TRUE and run with
> secure='yes', so only the firmware in SMM mode can write to flash.
> 
> Or you run with both code and vars read-only.
> Easiest is <loader>OVMF.fd</loader>.
>

Thanks for your suggestion! It's really helpful! I will try it.
 
> Or you disable secure boot (SECURE_BOOT_ENABLE=FALSE) in your
> builds.  You still have unprotected writable flash then, but
> it isn't a security hole any more.  And the assert isn't triggered
> either because that code path is only executed for secure boot
> builds.
>

Yes, before I produce the patch, I need to disable SECURE_BOOT_ENABLE
to workaround my VM hang problem.

IMHO, using "variable header State was invalid" assert to prevent user
writes to a unprotected flash is not a good idea. It causes some problem:

- User's existing virtual machine can not boot/reboot after updated to
  edk2-stable202211 OVMF. VM just hangs there and doesn't have any hint.

- The VM still works in the first boot. User doesn't know that second boot
  will hangs because they are writing an unprotected writable flash.

- Even enabled debug log, we don't know what does "NvVarStore Variable header
  State was invalid." mean.

Thanks
Joey Lee


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#97373): https://edk2.groups.io/g/devel/message/97373
Mute This Topic: https://groups.io/mt/95656983/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-