On 1/3/24 16:11, Gerd Hoffmann wrote:
> 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.
Good point about reinit, but it might still needlessly cause the loss of
preexistent variables, so if we can avoid it easily, we should.
>
>> 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).
Oh I certainly don't imply that we should reverse the order of the
checks; I meant to say (a) we should separate
VarHeaderEnd > VariableStoreHeader->Size
from
VarHeaderEnd == VariableStoreHeader->Size
and (b) we should perhaps consider the former condition (i.e.,
inequality) as a hard failure (and not as success), i.e., cause for
reformatting the varstore.
>
>> (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 have two comments here:
- if you are right, then I agree it's a good argument for keeping the
two conditions
VarHeaderEnd > VariableStoreHeader->Size
VarHeaderEnd == VariableStoreHeader->Size
unified as
VarHeaderEnd >= VariableStoreHeader->Size
*but* then it only strengthens my argument that the *handling* for this
case should not be a "break" statement, but "return EFI_NOT_FOUND"! (And
then the only successful exit from the loop would be for (StartId !=
0x55aa).)
- Do we know for sure that VAR_HEADER_VALID_ONLY is never expected to be
seen? What if the variable update design defines VAR_HEADER_VALID_ONLY
specifically so that the variable driver can recover from a power loss
"in the middle"? In that case, we should not consider
VAR_HEADER_VALID_ONLY reason for reformatting the whole varstore -- in
fact, the swith statement at the end of the patch tolaretes
VAR_HEADER_VALID_ONLY. So I figure, if we accept VAR_HEADER_VALID_ONLY
in that logic, then we should also accept VAR_HEADER_VALID_ONLY if it's
at the very end of the varstore.
>
>> 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 ...
OK!
>
>> (6) I suggest two further checks (within the braces here):
>>
>> - enforce
>>
>> VarHeader->NameSize > 0
>
> NameSize >= 4 ? (room for one char and the terminating null)
Sure, that works too.
>
>> - 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.
Good point -- I think I briefly considered it, but ruled it out because
<guid>:"<unknown>" in the log didn't look useful to me at once. But now
you're making me reconsider -- it simplifies the code, and it doesn't
"hurt" in the log either.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113168): https://edk2.groups.io/g/devel/message/113168
Mute This Topic: https://groups.io/mt/103171811/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
