Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables

["Gerd Hoffmann" <kraxel@redhat.com>]

  Hi,

> Second (and worse): the bug. In "OvmfPkg/RiscVVirt/VarStore.fdf.inc", it
> turns out that we *still* generate the gEfiVariableGuid varstore header
> signature, in case SECURE_BOOT_ENABLE is FALSE. For some reason, commit
> 92b27c2e6ada ("OvmfPkg/RiscVVirt: Add build files for Qemu Virt
> platform", 2023-02-16) did not consider commit d92eaabefbe0 ("OvmfPkg:
> simplify VARIABLE_STORE_HEADER generation", 2016-02-15), and
> *resurrected* the non-unified varstore generation for RiscVVirt.
> Furthermore, RiscVVirt uses "VirtNorFlashDxe" as its platform flash
> driver. As a result, if you now build RiscVVirt with this patch applied,
> and with SECURE_BOOT_ENABLE=FALSE, I expect the ValidateFvHeader()
> function to always fail, becase it will try to validate the contents of
> the varstore through AUTHENTICATED_VARIABLE_HEADER entries, despite the
> varstore containing (arguably valid) VARIABLE_HEADER entries.

I expect it will fail only once.  In case the checks don't pass
VirtNorFlashDxe will re-initialize the flash varstore with
gEfiAuthenticatedVariableGuid, so on next boot everything is fine.

> So here's what I propose:
> 
> - keep this patch, but *prepend* two other patches:
> 
> - first, reflect commit d92eaabefbe0 to
> "OvmfPkg/RiscVVirt/VarStore.fdf.inc" -- only generate the authenticated
> signature GUID, regardless of SECURE_BOOT_ENABLE,
> 
> - second, in this function, stop accepting the "gEfiVariableGuid"
> varstore header signature.

Makes sense.

> > +    if (VarHeaderEnd >= VariableStoreHeader->Size) {
> > +      DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __func__));
> > +      break;
> > +    }
> 
> (4) In case of inequality, the variable header is truncated. Accepting
> it as "success" looks doubtful.

We don't know whenever it is supposed to be a valid header, we didn't
check the StartId yet.

Reversing the check ordering looks wrong too (looking at header fields
before we know the header is inside the store).

> (5) In case of equality, the variable header fits, but it is followed by
> no payload at all. I think there are sub-cases to distinguish there:
> 
> - if the StartId differs from 0x55aa, then we may consider the variable
> list to be terminated, and break out of the loop (returning success from
> the function)
> 
> - if the StartId is 0x55aa, then we need to look further, beause we
> can't decide yet. For example, if State is VAR_HEADER_VALID_ONLY (0x7f),
> then it might be fine for the variable header (at the very end of the
> varstore) *not* to be followed by payload bytes (name, data).

Not sure this makes sense.  VAR_HEADER_VALID_ONLY is a temporary state,
while the variable driver writes name and data just after the header,
to be updated to VAR_ADDED when the write completed successfully.  So
I'd expect to never find a header without space for name + data.

> I find this code hard to review because I don't know (and the Intel
> whitepaper doesn't seem to tell) precisely how a valid variable list is
> supposed to be terminated.

Which is why the code logs the condition why it considers the list to be
terminated ...

> (6) I suggest two further checks (within the braces here):
> 
> - enforce
> 
>   VarHeader->NameSize > 0

NameSize >= 4 ?  (room for one char and the terminating null)

> - enforce
> 
>   VarName[VarHeader->NameSize / 2 - 1] == L'\0'

ok

> (This is also important for the immediately subsequent code: we print
> the name!)

Indeed.

> (7) Not really important, I'm just throwing it out: how about logging
> "VarHeader->VendorGuid" too?
> 
> It would require something like this:
> 
>   CONST EFI_GUID  *VarGuid;
> 
>   ...
> 
>   VarGuid = &gZeroGuid;
>   if (VarName == NULL) {
>     ...
>     VarGuid = &VarHeader->VendorGuid;
>     ...
>   }

I think we can just use VarHeader->VendorGuid directly, given that the
guid is part of the fixed header it should be valid even in case the
state is VAR_HEADER_VALID_ONLY.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113100): https://edk2.groups.io/g/devel/message/113100
Mute This Topic: https://groups.io/mt/103171811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-