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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
