On 12/7/23 10:44, Gerd Hoffmann wrote:
> Extend the ValidateFvHeader function, additionally to the header checks
> walk over the list of variables and sanity check them.
>
> In case we find inconsistencies indicating variable store corruption
> return EFI_NOT_FOUND so the variable store will be re-initialized.
>
> Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
> ---
> OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 82 +++++++++++++++++++++--
> 1 file changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> index 5ee98e9b595a..1bfb14495abd 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c
> @@ -185,11 +185,16 @@ ValidateFvHeader (
> IN NOR_FLASH_INSTANCE *Instance
> )
> {
> - UINT16 Checksum;
> - EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> - VARIABLE_STORE_HEADER *VariableStoreHeader;
> - UINTN VariableStoreLength;
> - UINTN FvLength;
> + UINT16 Checksum;
> + EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader;
> + VARIABLE_STORE_HEADER *VariableStoreHeader;
> + UINTN VarOffset;
> + AUTHENTICATED_VARIABLE_HEADER *VarHeader;
> + UINTN VarSize;
> + CHAR16 *VarName;
> + CHAR8 *VarState;
> + UINTN VariableStoreLength;
> + UINTN FvLength;
>
> FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddress;
>
> @@ -260,6 +265,73 @@ ValidateFvHeader (
> return EFI_NOT_FOUND;
> }
>
> + // check variables
(1) Comment style -- should be preceded and succeeded by otherwise empty //
lines
> + DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__));
> + VarOffset = sizeof (*VariableStoreHeader);
> + while (VarOffset + sizeof (*VarHeader) < VariableStoreHeader->Size) {
(2) The addition is not checked for overflow. When it is evaluated for the
first time, it cannot overflow, but I can't easily find a proof that it cannot
overflow in further iterations on 32-bit platforms.
(The strict "<" relop in itself seems justified; we always expect *some* data
after the variable header.)
I suggest rewriting as follows:
for (;;) {
RETURN_STATUS Status;
UINTN VarHeaderEnd;
Status = SafeUintnAdd (VarOffset, sizeof (*VarHeader), &VarHeaderEnd);
if (RETURN_ERROR (Status)) {
return EFI_NOT_FOUND;
}
if (VarHeaderEnd >= VariableStoreHeader->Size) {
break;
}
Perhaps even the second check should return EFI_NOT_FOUND, assuming we require
(StartId != 0x55aa) as the only successful termination condition.
> + VarHeader = (VOID *)((UINTN)VariableStoreHeader + VarOffset);
> + if (VarHeader->StartId != 0x55aa) {
> + DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__));
> + break;
> + }
> +
> + VarSize = sizeof (*VarHeader) + VarHeader->NameSize +
> VarHeader->DataSize;
(3) Given that we expect that the varstore may be corrupt, this addition can
definitely overflow on 32-bit platforms (where the additions are performed in
UINT32). I suggest using SafeUintnAdd() from SafeIntLib again.
> + if (VarOffset + VarSize > VariableStoreHeader->Size) {
(4) This addition should be checked for overflow too.
Alternatively -- given that, from previous checks, here we know for sure that
VarOffset is strictly smaller than VariableStoreHeader->Size --, the expression
can be rearranged as follows (by subtracting VarOffset from both sides):
if (VarSize > VariableStoreHeader->Size - VarOffset) {
Mathematically this means the same thing, but the subtraction on the right hand
side cannot underflow.
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: invalid variable size: 0x%x + 0x%x + 0x%x + 0x%x > 0x%x\n",
> + __func__,
> + VarOffset,
(5) VarOffset is of type UINTN, which may be UINT32 or UINT64. %x is not a
proper format specifier for printing UINT64.
PrintLib offers no format specifier for UINTN, only for UINT32 (%x, %u) and
UINT64 (%lx / %Lx, %lu / %Lu).
Therefore, the best way to format a UINTN value is:
- convert it to UINT64 explicitly,
- print it with one of the format specifiers matching UINT64.
For example,
DEBUG ((DEBUG_ERROR, "%Lx\n", (UINT64)VarOffset));
> + sizeof (*VarHeader),
(6) same issue, sizeof evals to a UINTN.
> + VarHeader->NameSize,
> + VarHeader->DataSize,
> + VariableStoreHeader->Size
these are good (all three are UINT32)
> + ));
> + return EFI_NOT_FOUND;
> + }
> +
> + VarName = (VOID *)((UINTN)VariableStoreHeader + VarOffset
> + + sizeof (*VarHeader));
(7) This seems premature. We should first check that NameSize is (a) nonzero
and (b) divisible by 2.
> + switch (VarHeader->State) {
> + case VAR_HEADER_VALID_ONLY:
> + VarState = "header-ok";
(8) Then better make VarState point to a CONST CHAR8, not just a CHAR8.
> + VarName = L"<unknown>";
(9) Ditto.
(10) If VarName is invalid here when State is VAR_HEADER_VALID_ONLY, then we
should calculate VarName only when the State says it's going to be valid.
Creating and invalid pointer value first and then overwriting it with something
else is "cart before horse".
> + break;
> + case VAR_ADDED:
> + VarState = "ok";
> + break;
> + case VAR_ADDED &VAR_IN_DELETED_TRANSITION:
> + VarState = "del-in-transition";
> + break;
> + case VAR_ADDED &VAR_DELETED:
> + case VAR_ADDED &VAR_DELETED &VAR_IN_DELETED_TRANSITION:
> + VarState = "deleted";
> + break;
> + default:
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: invalid variable state: 0x%x\n",
> + __func__,
> + VarHeader->State
> + ));
This is fine. The State field is UINT8, so it's getting promoted to "int"
(INT32). "%x" is fine for printing UINT32 ("unsigned int"), and it's fine to
reinterpret nonnegative "int"s as "unsigned int"s, per C std.
> + return EFI_NOT_FOUND;
> + }
> +
> + DEBUG ((
> + DEBUG_VERBOSE,
> + "%a: +0x%04x: name=0x%x data=0x%x '%s' (%a)\n",
> + __func__,
> + VarOffset,
(10) same comment as (5)
> + VarHeader->NameSize,
> + VarHeader->DataSize,
> + VarName,
(11) regarding VarName, we have not checked whether it was NUL-terminated; %s
relies on that.
Alternatively, you can provide the precision as a separate UINTN argument:
DEBUG ((DEBUG_VERBOSE, "%.*s\n", (UINTN)(VarHeader->NameSize / 2), VarName));
> + VarState
> + ));
> +
> + VarSize = (VarSize + 3) & ~3; // align
Two problems:
(12) Unchecked addition.
(13) The bitwise negation operator applied to a signed integer is exceeding
ugly:
(a) It inverts the sign bit, which *happens* to produce a negative value.
(b) That negative value is then converted to UINTN (for the bitwise AND), by
adding -- mathematically speaking -- (MAX_UINTN+1) to it. This only produces
the expected result because the original negative value uses two's complement
representation in the first place.
It's (almost) better to use the ALIGN_VALUE() macro from
"MdePkg/Include/Base.h"; but that too would not check for overflow. Suggestion:
UINTN LowBits;
LowBits = VarSize & (BIT0 | BIT1);
if (LowBits > 0) {
Status = SafeUintnAdd (VarSize, BIT2 - LowBits, &VarSize);
if (RETURN_ERROR (Status)) {
return EFI_NOT_FOUND;
}
}
(14) However, even *this* should be performed at the top, where we check
(VarOffset + VarSize > VariableStoreHeader->Size). More precisely, that check
should consider the aligned-up variable size. That's because:
> + VarOffset += VarSize;
this addition here uses the aligned-up VarSize as increment, which may be
larger than the previously checked VarSize value -- and so it can land us
outside of VariableStoreHeader->Size at once.
> + }
> +
> return EFI_SUCCESS;
> }
>
The general idea is, once we don't trust the varstore, there cannot be a
*single* unchecked addition in the code. (Unless we can *prove* that overflow
is impossible.)
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112340): https://edk2.groups.io/g/devel/message/112340
Mute This Topic: https://groups.io/mt/103031342/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
