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 BB50F941490 for ; Thu, 4 Jan 2024 13:21:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=1XD5Lk7UaWGBTcXYIwpuPMRxwSoGKw7TNNK1ryzewro=; 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=1704374468; v=1; b=jkVWSZOOG9agldJ9H9jNY66MLTlx6r4+uD854xI+RWmS+rxndYa2D7+N/a+JTXUooOmyvz3j 4Ide+3v3mJ6cIZnHwIMrG28nHKz4U7OAOWLYhNcszyLaREPKmcRh+3OTEsISMW4T0FfX/286ZWG 8Km+lReDOekv9Une8GuZz+jE= X-Received: by 127.0.0.2 with SMTP id SylKYY7687511xDEC5EzLZq6; Thu, 04 Jan 2024 05:21:08 -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.53488.1704374467681373085 for ; Thu, 04 Jan 2024 05:21:07 -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-247-Zmaioqg7Ns2ZrimlSRARsw-1; Thu, 04 Jan 2024 08:21:05 -0500 X-MC-Unique: Zmaioqg7Ns2ZrimlSRARsw-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (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 C458D185A780; Thu, 4 Jan 2024 13:21:04 +0000 (UTC) X-Received: from [10.39.194.24] (unknown [10.39.194.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 71F9B492BE6; Thu, 4 Jan 2024 13:21:03 +0000 (UTC) Message-ID: Date: Thu, 4 Jan 2024 14:21:00 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables To: Gerd Hoffmann Cc: devel@edk2.groups.io, Ard Biesheuvel , oliver@redhat.com, mike.maslenkin@gmail.com, Jiewen Yao References: <20231214153156.46812-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 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: G9QeMHuzyFHf8Zbou5Y9aC5Kx7686176AA= 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=jkVWSZOO; 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 1/3/24 16:11, Gerd Hoffmann wrote: > Hi, >=20 >> 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. >=20 > 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. >=20 >> 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. >=20 > Makes sense. >=20 >>> + if (VarHeaderEnd >=3D VariableStoreHeader->Size) { >>> + DEBUG ((DEBUG_INFO, "%a: end of var list (no space left)\n", __f= unc__)); >>> + break; >>> + } >> >> (4) In case of inequality, the variable header is truncated. Accepting >> it as "success" looks doubtful. >=20 > We don't know whenever it is supposed to be a valid header, we didn't > check the StartId yet. >=20 > 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 =3D=3D 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. >=20 >> (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). >=20 > 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 =3D=3D VariableStoreHeader->Size unified as VarHeaderEnd >=3D 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 !=3D 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. >=20 >> 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. >=20 > Which is why the code logs the condition why it considers the list to be > terminated ... OK! >=20 >> (6) I suggest two further checks (within the braces here): >> >> - enforce >> >> VarHeader->NameSize > 0 >=20 > NameSize >=3D 4 ? (room for one char and the terminating null) Sure, that works too. >=20 >> - enforce >> >> VarName[VarHeader->NameSize / 2 - 1] =3D=3D L'\0' >=20 > ok >=20 >> (This is also important for the immediately subsequent code: we print >> the name!) >=20 > Indeed. >=20 >> (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; >> ... >> } >=20 > 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 :"" 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 -=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 (#113168): https://edk2.groups.io/g/devel/message/113168 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-