From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 4F7DAAC0E30 for ; Wed, 3 Jan 2024 12:56:34 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=LvIzZ1dg2jP2o+cEUeuQFib4K9LixTVf3ZFbyq06cjg=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1704286592; v=1; b=wGbOreItpTNT4+ESy6TJstgRlBXRigfm8z2dk4Omg9ib9ckrMgMsDBhZg7iUPD+JllGa2Lf4 Z4SctdUgoRaDucjBpbqfHqOSEmv0REwxlMNDTDDUh92og/IJFZTXYQ6LiEZI72xkdBucfLlyvzc hIABaNdLyHFLKsdZCzcLtkss= X-Received: by 127.0.0.2 with SMTP id 1fmpYY7687511xPgNdhf1A0k; Wed, 03 Jan 2024 04:56:32 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.16695.1704286591993120896 for ; Wed, 03 Jan 2024 04:56:32 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-549-D7R5c8l_OEGSuTMlP18spg-1; Wed, 03 Jan 2024 07:56:28 -0500 X-MC-Unique: D7R5c8l_OEGSuTMlP18spg-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B3104185A780; Wed, 3 Jan 2024 12:56:27 +0000 (UTC) X-Received: from [10.39.193.243] (unknown [10.39.193.243]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 71F282026D66; Wed, 3 Jan 2024 12:56:26 +0000 (UTC) Message-ID: Date: Wed, 3 Jan 2024 13:56:24 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables To: devel@edk2.groups.io, kraxel@redhat.com Cc: Ard Biesheuvel , oliver@redhat.com, mike.maslenkin@gmail.com, Jiewen Yao References: <20231214153156.46812-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20231214153156.46812-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 9y1ttDqC4qDGgtdgRY3LNc76x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=wGbOreIt; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 12/14/23 16:31, 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 > --- > OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf | 1 + > OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 125 +++++++++++++++++++- > 2 files changed, 121 insertions(+), 5 deletions(-) > > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf b/OvmfPkg/VirtNo= rFlashDxe/VirtNorFlashDxe.inf > index 2a3d4a218e61..f549400280a1 100644 > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf > @@ -34,6 +34,7 @@ [LibraryClasses] > DxeServicesTableLib > HobLib > IoLib > + SafeIntLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorF= lashDxe/VirtNorFlashFvb.c > index 5ee98e9b595a..d69bf8783d77 100644 > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c > @@ -12,6 +12,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -185,11 +186,19 @@ 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; > + UINTN VarHeaderEnd; > + UINTN VarNameEnd; > + UINTN VarEnd; > + AUTHENTICATED_VARIABLE_HEADER *VarHeader; (1) The fact that we try to traverse the variables using AUTHENTICATED_VARIABLE_HEADER creates a *very slight* inconsistency in the code. More precisely, the inconsistency is already there, but this patch makes it "stronger". Or even more precisely... this patch in fact introduces a bug, as far as I can tell, for the RISC-V virt platform firmware. Here's the details: In ancient times, authenticated and non-authenticated variable stores were handled by separate variable drivers. Over time they got unified -- in commit fa0737a839d0 ("MdeModulePkg Variable: Merge from Auth Variable driver in SecurityPkg", 2015-07-01). The unified driver would then manage both kinds of varstores. This got reflected under OvmfPkg in commit d92eaabefbe0 ("OvmfPkg: simplify VARIABLE_STORE_HEADER generation", 2016-02-15). I recommend reviewing *that* commit in detail. ArmVirtPkg's varstore generation was implemented in commit bf57a42a0e2c ("ArmVirtPkg: add FDF definition for empty varstore", 2016-06-23), duplicating the (simplified, auth-only) varstore header signature from OvmfPkg. The upshot was that the OVMF and ArmVirt build processes would only generate varstores with "gEfiAuthenticatedVariableGuid" in the varstore header signature. At the time of commit bf57a42a0e2c ("ArmVirtPkg: add FDF definition for empty varstore", 2016-06-23), we didn't have VirtNorFlashDxe yet. The various platform flash drivers back then tolerated (recognized) the "gEfiVariableGuid" varstore header signature as well. So it's pretty difficult to say now whether the exclusive generation of the "gEfiAuthenticatedVariableGuid" signature in OVMF and ArmVirt *should have been followed* of a restriction of the ValidateFvHeader() functions in all those drivers -- i.e., whether we should have updated all those drivers too, to reject "gEfiVariableGuid". After all, the unified variable driver would just deal with "gEfiVariableGuid" fine, so the "tolerance" of "gEfiVariableGuid" in the flash drivers' validation code was harmless. But that's not the case anymore, with this patch: For the variable traversal, we now expect each variable to come with an AUTHENTICATED_VARIABLE_HEADER. First, that is inconsistent with our tolerance of "gEfiVariableGuid" in the varstore header signature. If one part of the code says "OK, this can validly be 'gEfiVariableGuid'", then the other part of the code should read the variable headers *as* VARIABLE_HEADER. Or else, in the opposite direction: if we're only willing to parse the variable list via AUTHENTICATED_VARIABLE_HEADER, then we should now reject the "gEfiVariableGuid" header signature *upfront*. 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=3DFALSE, 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. 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. (2) Minor: consider CONST-ifying FwVolHeader, VariableStoreHeader, VarHeader. (Feel free to ignore if it causes many problems.) (3) Minor: consider declaring all of the new variables except VarOffset (i.e., the variables VarHeaderEnd, VarNameEnd, VarEnd, VarHeader, VarName, VarState, Status) at the top of the body of the "for" loop below. That restricts their scopes as narrowly as possible, which helps us understand the data flow. > + CHAR16 *VarName; > + CONST CHAR8 *VarState; > + UINTN VariableStoreLength; > + UINTN FvLength; > + RETURN_STATUS Status; > > FwVolHeader =3D (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddr= ess; > > @@ -260,6 +269,112 @@ ValidateFvHeader ( > return EFI_NOT_FOUND; > } > > + // > + // check variables > + // > + DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__)); > + VarOffset =3D sizeof (*VariableStoreHeader); > + for ( ; ;) { > + Status =3D SafeUintnAdd (VarOffset, sizeof (*VarHeader), &VarHeaderE= nd); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__)); > + return EFI_NOT_FOUND; > + } > + > + if (VarHeaderEnd >=3D VariableStoreHeader->Size) { > + DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __fun= c__)); > + break; > + } (4) In case of inequality, the variable header is truncated. Accepting it as "success" looks doubtful. (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). 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. > + > + VarHeader =3D (VOID *)((UINTN)VariableStoreHeader + VarOffset); This addition is unchecked, but it's safe. We're operating under the assumption that the complete variable store (starting at VariableStoreHeader, running for VariableStoreHeader->Size bytes) has been mapped into guest-physical address space. Furthermore, at the very end of the address space, the "code" flash device is mapped (with the reset vector at the tail). Therefore, as long as our relative offsets are smaller than, or equal to, VariableStoreHeader->Size, we can safely evaluate the *absolute addresses* (possibly pointing just one past the varstore, but still not overflowing, in case of equality), and if the relative offset is strictly smaller than VariableStoreHeader->Size, then we can even dereference those absolute addresses. > + if (VarHeader->StartId !=3D 0x55aa) { > + DEBUG ((DEBUG_INFO, "%a: end of var list (no startid)\n", __func__= )); > + break; > + } > + > + VarName =3D NULL; > + switch (VarHeader->State) { > + // usage: State =3D VAR_HEADER_VALID_ONLY > + case VAR_HEADER_VALID_ONLY: > + VarState =3D "header-ok"; > + VarName =3D L""; > + break; > + > + // usage: State =3D VAR_ADDED > + case VAR_ADDED: > + VarState =3D "ok"; > + break; > + > + // usage: State &=3D VAR_IN_DELETED_TRANSITION > + case VAR_ADDED &VAR_IN_DELETED_TRANSITION: > + VarState =3D "del-in-transition"; > + break; > + > + // usage: State &=3D VAR_DELETED > + case VAR_ADDED &VAR_DELETED: > + case VAR_ADDED &VAR_DELETED &VAR_IN_DELETED_TRANSITION: > + VarState =3D "deleted"; > + break; > + > + default: > + DEBUG (( > + DEBUG_ERROR, > + "%a: invalid variable state: 0x%x\n", > + __func__, > + VarHeader->State > + )); > + return EFI_NOT_FOUND; > + } > + > + Status =3D SafeUintnAdd (VarHeaderEnd, VarHeader->NameSize, &VarName= End); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__)); > + return EFI_NOT_FOUND; > + } > + > + Status =3D SafeUintnAdd (VarNameEnd, VarHeader->DataSize, &VarEnd); > + if (RETURN_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__)); > + return EFI_NOT_FOUND; > + } > + > + if (VarEnd > VariableStoreHeader->Size) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: invalid variable size: 0x%Lx + 0x%Lx + 0x%x + 0x%x > 0x%x\n= ", > + __func__, > + (UINT64)VarOffset, > + (UINT64)(sizeof (*VarHeader)), > + VarHeader->NameSize, > + VarHeader->DataSize, > + VariableStoreHeader->Size > + )); > + return EFI_NOT_FOUND; > + } > + > + if (VarHeader->NameSize & 1) { > + DEBUG ((DEBUG_ERROR, "%a: invalid name size\n", __func__)); > + return EFI_NOT_FOUND; > + } > + > + if (VarName =3D=3D NULL) { > + VarName =3D (VOID *)((UINTN)VariableStoreHeader + VarHeaderEnd); >From the perspective of "VarName" being well-aligned, I think this is fine. Our assumption thus far is that VarHeader is well-aligned, and given the sizeof AUTHENTICATED_VARIABLE_HEADER, VarName should be well-aligned as well. (6) I suggest two further checks (within the braces here): - enforce VarHeader->NameSize > 0 - enforce VarName[VarHeader->NameSize / 2 - 1] =3D=3D L'\0' because the "AUTHENTICATED_VARIABLE_HEADER.NameSize" documentation in "MdeModulePkg/Include/Guid/VariableFormat.h" specifies NUL-termination. (This is also important for the immediately subsequent code: we print the name!) > + } > + > + DEBUG (( > + DEBUG_VERBOSE, > + "%a: +0x%04Lx: name=3D0x%x data=3D0x%x '%s' (%a)\n", > + __func__, > + (UINT64)VarOffset, > + VarHeader->NameSize, > + VarHeader->DataSize, > + VarName, > + VarState > + )); (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 =3D &gZeroGuid; if (VarName =3D=3D NULL) { ... VarGuid =3D &VarHeader->VendorGuid; ... } and then format VarGuid with "%g" in the DEBUG macro invocation. This would be useful for the non-standard namespace GUIDs especially. > + > + VarOffset =3D ALIGN_VALUE (VarEnd, 4); (8) Apologies if it was me who suggested ALIGN_VALUE() previously, but this is, in effect, an unchecked addition. I can't off-hand see evidence that it can never overflow (the previous checks don't seem to prevent an overflow here), so I suggest: // // the next variable header starts aligned at 4 bytes // Status =3D SafeUintnAdd (VarEnd, (4 - (VarEnd & 4)) & 4, &VarOffset); if RETURN_ERROR (Status) { DEBUG ((DEBUG_ERROR, "%a: integer overflow\n", __func__)); return EFI_NOT_FOUND; } > + } > + > return EFI_SUCCESS; > } > Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113084): https://edk2.groups.io/g/devel/message/113084 Mute This Topic: https://groups.io/mt/103171811/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-