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 1212DAC1BA3 for ; Mon, 11 Dec 2023 23:25:47 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=n+wD2HK1jNn5qlpyvAxXQfek+M6e+s0xGOG/AlIM6zo=; 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=1702337146; v=1; b=YdTukvEK+ngyPBgLllvpiKkM1DwMhD03cX4QU2+LtODBcvmKJDzJmaDyzUXQD5T+Mb4P7wlg Us3pIRvVVVy3nxEl7O0aF0HLNxqcadWk8BjT8HmMC5PxyBDhEQR7pxf5tBpdUGLmaf+ooYxxYDC t/TyNemau+r5htCxRO9ay+4o= X-Received: by 127.0.0.2 with SMTP id ZcQaYY7687511xRGrnQMgz14; Mon, 11 Dec 2023 15:25:46 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.6212.1702337146052810417 for ; Mon, 11 Dec 2023 15:25:46 -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-130-Ht_0uIdyObGbRiBqbDr-aw-1; Mon, 11 Dec 2023 18:25:42 -0500 X-MC-Unique: Ht_0uIdyObGbRiBqbDr-aw-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (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 DF618833B42; Mon, 11 Dec 2023 23:25:39 +0000 (UTC) X-Received: from [10.39.192.110] (unknown [10.39.192.110]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AE33C2166B32; Mon, 11 Dec 2023 23:25:38 +0000 (UTC) Message-ID: Date: Tue, 12 Dec 2023 00:25:37 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables To: devel@edk2.groups.io, kraxel@redhat.com Cc: mike.maslenkin@gmail.com, Jiewen Yao , Ard Biesheuvel , oliver@redhat.com References: <20231207094404.270381-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20231207094404.270381-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 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: ZcEg2qdYizWv0RBLjBvUD5XRx7686176AA= 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=YdTukvEK; 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/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. >=20 > In case we find inconsistencies indicating variable store corruption > return EFI_NOT_FOUND so the variable store will be re-initialized. >=20 > Signed-off-by: Gerd Hoffmann > --- > OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 82 +++++++++++++++++++++-- > 1 file changed, 77 insertions(+), 5 deletions(-) >=20 > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c b/OvmfPkg/VirtNorF= lashDxe/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; > =20 > FwVolHeader =3D (EFI_FIRMWARE_VOLUME_HEADER *)Instance->RegionBaseAddr= ess; > =20 > @@ -260,6 +265,73 @@ ValidateFvHeader ( > return EFI_NOT_FOUND; > } > =20 > + // check variables (1) Comment style -- should be preceded and succeeded by otherwise empty //= lines > + DEBUG ((DEBUG_INFO, "%a: checking variables\n", __func__)); > + VarOffset =3D 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 can= not overflow in further iterations on 32-bit platforms. (The strict "<" relop in itself seems justified; we always expect *some* da= ta after the variable header.) I suggest rewriting as follows: for (;;) { RETURN_STATUS Status; UINTN VarHeaderEnd; Status =3D SafeUintnAdd (VarOffset, sizeof (*VarHeader), &VarHeaderEnd)= ; if (RETURN_ERROR (Status)) { return EFI_NOT_FOUND; } if (VarHeaderEnd >=3D VariableStoreHeader->Size) { break; } Perhaps even the second check should return EFI_NOT_FOUND, assuming we requ= ire (StartId !=3D 0x55aa) as the only successful termination condition. > + VarHeader =3D (VOID *)((UINTN)VariableStoreHeader + VarOffset); > + if (VarHeader->StartId !=3D 0x55aa) { > + DEBUG ((DEBUG_INFO, "%a: end of var list\n", __func__)); > + break; > + } > + > + VarSize =3D sizeof (*VarHeader) + VarHeader->NameSize + VarHeader->D= ataSize; (3) Given that we expect that the varstore may be corrupt, this addition ca= n definitely overflow on 32-bit platforms (where the additions are performe= d 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 th= at VarOffset is strictly smaller than VariableStoreHeader->Size --, the exp= ression can be rearranged as follows (by subtracting VarOffset from both si= des): 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 =3D (VOID *)((UINTN)VariableStoreHeader + VarOffset > + + sizeof (*VarHeader)); (7) This seems premature. We should first check that NameSize is (a) nonzer= o and (b) divisible by 2. > + switch (VarHeader->State) { > + case VAR_HEADER_VALID_ONLY: > + VarState =3D "header-ok"; (8) Then better make VarState point to a CONST CHAR8, not just a CHAR8. > + VarName =3D L""; (9) Ditto. (10) If VarName is invalid here when State is VAR_HEADER_VALID_ONLY, then w= e 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 some= thing else is "cart before horse". > + break; > + case VAR_ADDED: > + VarState =3D "ok"; > + break; > + case VAR_ADDED &VAR_IN_DELETED_TRANSITION: > + VarState =3D "del-in-transition"; > + break; > + 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 > + )); 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=3D0x%x data=3D0x%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), VarNam= e)); > + VarState > + )); > + > + VarSize =3D (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), b= y adding -- mathematically speaking -- (MAX_UINTN+1) to it. This only produ= ces the expected result because the original negative value uses two's comp= lement representation in the first place. It's (almost) better to use the ALIGN_VALUE() macro from "MdePkg/Include/Ba= se.h"; but that too would not check for overflow. Suggestion: UINTN LowBits; LowBits =3D VarSize & (BIT0 | BIT1); if (LowBits > 0) { Status =3D 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 (V= arOffset + VarSize > VariableStoreHeader->Size). More precisely, that check= should consider the aligned-up variable size. That's because: > + VarOffset +=3D VarSize; this addition here uses the aligned-up VarSize as increment, which may be l= arger than the previously checked VarSize value -- and so it can land us ou= tside of VariableStoreHeader->Size at once. > + } > + > return EFI_SUCCESS; > } > =20 The general idea is, once we don't trust the varstore, there cannot be a *s= ingle* unchecked addition in the code. (Unless we can *prove* that overflow= is impossible.) 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 (#112340): https://edk2.groups.io/g/devel/message/112340 Mute This Topic: https://groups.io/mt/103031342/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-