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 69D6EAC13C4 for ; Fri, 8 Dec 2023 12:04:57 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=3SCvaGh+BYlxzS79jF4pfFnHsLIbCWSlNFrMpj0HKcE=; 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=1702037095; v=1; b=HAx3tWcQBhGFsda+lojtE5puYFRmALcopJyn8zSX9/YLliVB6vvHJ/j9H+VPSeDexQ16QJIA 06v1a6bG89+wBxsijfRy/qhPprTzmXLVEDC/uhlcqXb7og1IRz/syz3mLxSOWX5RZD0xDekdi/p VROoaVAgnnb/XStsq23r+j7c= X-Received: by 127.0.0.2 with SMTP id uIUzYY7687511xws1Wo0Gjhq; Fri, 08 Dec 2023 04:04:55 -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.30809.1702037095128613477 for ; Fri, 08 Dec 2023 04:04:55 -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-688-GzqBJ7KTMoyac5_SBkSm8Q-1; Fri, 08 Dec 2023 07:04:51 -0500 X-MC-Unique: GzqBJ7KTMoyac5_SBkSm8Q-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (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 F25FC185A782; Fri, 8 Dec 2023 12:04:50 +0000 (UTC) X-Received: from dobby.home.kraxel.org (unknown [10.39.194.15]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B970CC157C0; Fri, 8 Dec 2023 12:04:50 +0000 (UTC) X-Received: by dobby.home.kraxel.org (Postfix, from userid 1000) id C9F9681B4C; Fri, 8 Dec 2023 13:04:46 +0100 (CET) Date: Fri, 8 Dec 2023 13:04:46 +0100 From: "Gerd Hoffmann" To: Ard Biesheuvel Cc: devel@edk2.groups.io, mike.maslenkin@gmail.com, Jiewen Yao , Ard Biesheuvel , oliver@redhat.com Subject: Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/VirtNorFlashDxe: sanity-check variables Message-ID: References: <20231207094404.270381-1-kraxel@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 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: ShWWwG1GDaovPbVHRza6YwA8x7686176AA= 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=HAx3tWcQ; 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 Thu, Dec 07, 2023 at 05:16:10PM +0100, Ard Biesheuvel wrote: > Hi Gerd, > > On Thu, 7 Dec 2023 at 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 > > This seems like a good idea to me, and the change looks generally > fine, except for one nit below: > > + case VAR_HEADER_VALID_ONLY: > > + VarState = "header-ok"; > > + VarName = L""; > > + break; > > + case VAR_ADDED: > > + VarState = "ok"; > > + break; > > + case VAR_ADDED &VAR_IN_DELETED_TRANSITION: > > This looks odd, so please add a comment explaining why these constants > are constructed this way. The VAR_HEADER_VALID_ONLY and VAR_ADDED states are applied using an assignment, i.e. "State = VAR_...". The other two are used to clear bits, i.e. "State &= VAR_...". So the usual livecycle for the State field is: (1) 0x7f (VAR_HEADER_VALID_ONLY) (2) 0x3f (VAR_ADDED) (3) 0x3e (VAR_ADDED & VAR_IN_DELETED_TRANSITION) (4) 0x3c (VAR_ADDED & VAR_IN_DELETED_TRANSITION & VAR_DELETED) Seems it can also happen that variable entries jump directly from (2) to (4), resulting in 0x3d (VAR_ADDED & VAR_DELETED) being an possible alternative value for (4). I'm not fully sure what happens to VAR_HEADER_VALID_ONLY entries, i.e. whenever they go through the (3) + (4) deletion cycles too or whenever they are garbage-collected in some other way. I suspect the latter, given that the reclaim logic in the variable driver looks at the variable names, but VAR_HEADER_VALID_ONLY entries don't have a valid variable name. BTW: Is there any good documentation on the design of the variable driver and the fault tolerant flash writes? Is the variable flash supposed to be in a consistent state at any given time, even if interrupted in the middle of some update operation? The qemu flash emulation is a bit sloppy, specifically block writes can be half-completed when a reset happens in the middle of the operation. Which maybe is the root cause for varstore corruption. On the other hand the edk2 flash code does not look like it carefully chooses blocks when updating flash, so I'm not sure it actually depends on atomic block updates to ensure consistency. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112226): https://edk2.groups.io/g/devel/message/112226 Mute This Topic: https://groups.io/mt/103031342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-