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 EBC587803E7 for ; Wed, 3 Jan 2024 15:11:33 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ZiBXnLf1smzVhpfxuYb4gCW2es2vTTQeuqQJAYTYrDQ=; c=relaxed/simple; d=groups.io; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1704294692; v=1; b=RPOGKV+B5zLqFQwk8rBqDwYVwSH2j6uSXVbdpaAr3/hegTB7lPzXf/FlvHKOPWqQF6lE/Y/N FkM7XhnTeVOQCvlB34dsW6XQa/e+vAo0eqhOvhVq1b7EIhbo2AOw3eqH6dWdD+fhssYYVWcbc50 4mTCMnuG0V1WBR5nPa9s1+y0= X-Received: by 127.0.0.2 with SMTP id m8VtYY7687511xfJPmFUbShs; Wed, 03 Jan 2024 07:11:32 -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.web10.19469.1704294691941908044 for ; Wed, 03 Jan 2024 07:11: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-582-Lj3b0U0KOq21ECEqSq_Ulg-1; Wed, 03 Jan 2024 10:11:26 -0500 X-MC-Unique: Lj3b0U0KOq21ECEqSq_Ulg-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 9CB33887188; Wed, 3 Jan 2024 15:11:25 +0000 (UTC) X-Received: from sirius.home.kraxel.org (unknown [10.39.193.182]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6C63E492BF0; Wed, 3 Jan 2024 15:11:25 +0000 (UTC) X-Received: by sirius.home.kraxel.org (Postfix, from userid 1000) id 3C62A1800605; Wed, 3 Jan 2024 16:11:24 +0100 (CET) Date: Wed, 3 Jan 2024 16:11:24 +0100 From: "Gerd Hoffmann" To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , oliver@redhat.com, mike.maslenkin@gmail.com, Jiewen Yao Subject: Re: [edk2-devel] [PATCH v3 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables Message-ID: References: <20231214153156.46812-1-kraxel@redhat.com> MIME-Version: 1.0 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,kraxel@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: rZZteRqli8NEXznNftmhVfZjx7686176AA= Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=RPOGKV+B; 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 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. > 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). > (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 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 ... > (6) I suggest two further checks (within the braces here): > > - enforce > > VarHeader->NameSize > 0 NameSize >= 4 ? (room for one char and the terminating null) > - 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. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113100): https://edk2.groups.io/g/devel/message/113100 Mute This Topic: https://groups.io/mt/103171811/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-