From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from fb.mail.gandi.net (spool8.mail.gandi.net [217.70.178.217]) by nmboxes7-ms7.sd4.0x35.net (Postfix) with ESMTPS id C2EB161930 for ; Tue, 16 Jan 2024 05:08:39 +0000 (UTC) Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by fb.mail.gandi.net (Postfix) with ESMTPS id 4FAF760377 for ; Tue, 16 Jan 2024 05:08:39 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=GWVFsx5AswK6f5qVlF0GK8rSDfN5biUIsM0P5Ym8JT8=; 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=1705381718; v=1; b=uYAmPVIiBAIGUW4bfxA53mfjbBuirqDHSYPOQyHUVVLSXgvSRALnsxDcrO5beSlzw8dE61QX hqvtHilPCgH3FE64Q/cMPVZF3FP1aAnA5OmHMK4KsG02P2dUGMewhAynWlLG4JlXyUGQVwcszMC y+nsxXE/BRE9DOk/OPo+uDeQ= X-Received: by 127.0.0.2 with SMTP id 5Cc6YY7687511x6sI4mvSTw1; Mon, 15 Jan 2024 21:08:38 -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.75099.1705314118044771188 for ; Mon, 15 Jan 2024 02:21:58 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-507-EdPeDeEEOvWLbNUyqNjxiA-1; Mon, 15 Jan 2024 05:21:48 -0500 X-MC-Unique: EdPeDeEEOvWLbNUyqNjxiA-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 9A8FF3C0ED47; Mon, 15 Jan 2024 10:21:48 +0000 (UTC) X-Received: from [10.39.193.170] (unknown [10.39.193.170]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 953B940C6EB9; Mon, 15 Jan 2024 10:21:47 +0000 (UTC) Message-ID: <01ac5fab-2710-4231-a7cd-1c550fa6319f@redhat.com> Date: Mon, 15 Jan 2024 11:21:46 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements To: devel@edk2.groups.io, kraxel@redhat.com Cc: Ard Biesheuvel , Jiewen Yao , oliver@redhat.com References: <20240112113754.14710-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20240112113754.14710-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: UE0gjdJhL7xQL9LSn2bM0wS8x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: fb.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=uYAmPVIi; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (fb.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 1/12/24 12:37, Gerd Hoffmann wrote: > This is a little series containing the flash corruption fix sent > yesterday with an slightly improved commit message and some small > improvements on top of this. > > Gerd Hoffmann (4): > OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads > OvmfPkg/VirtNorFlashDxe: clarify block write logic > OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase > OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too > > OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 33 +++++++++++------------ > OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c | 5 ++++ > 2 files changed, 21 insertions(+), 17 deletions(-) > Looking at the original code makes me throw a fit (no offense -- I don't know who wrote it, and I don't want to check). There is not a single diagram in the code, when that would be central to the whole thing. 0 128 256 [----------------|----------------] ^ ^ ^ | | | | | (Offset & 0x7F) + NumBytes; i.e., the Offset inside | | (or just past) the *double-word* such that Offset is | | the *exclusive* end of the (logical) update | | | Offset & 0x7F; i.e., Offset within the "word"; | this is where the (logical) update is supposed to start | Offset & ~(UINTN)0x7F; i.e., Offset truncated to "word" boundary In this diagram, NumBytes is already limited to 256; that's because of the existent condition if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <=3D (2 * P30_MAX_BUFF= ER_SIZE_IN_BYTES)) { So, independently of the bug in the code that this series is supposed to fix, some problems with the original code: - no diagram (see above) - rampant duplication of hard to understand expressions, such as: - Offset & ~BOUNDARY_OF_32_WORDS (side comment: applying the bit-neg on a *signed integer* deserves its own brown paper bag) - *NumBytes + (Offset & BOUNDARY_OF_32_WORDS) - Offset & ~BOUNDARY_OF_32_WORDS - more bit-neg applied to a *signed integer*: ~OrigData[CurOffset] because OrigData[CurOffset] is a UINT8, which gets promoted to INT32, and that's when the bit-neg is applied - when the second word write is deemed necessary, then the *BlockAddress* variable is bumped by 128 bytes out of laziness for said second write -- and that is a *semantic wreck*. The BlockAddress does not change *at all*; it's the start offset within the block that increases by 128 bytes for the second word write. - The weird Exit and DoErase labels are fugly. The function should either be split into two functions, or at least reorganized with "ifs" such that this jumping is not necessary. Gotos are fine, but only for error paths / cleanup on exit, not for business logic selection. IOW, the main offender is DoErase. Then comments on the patch set: - In my opinion, the series should progress in opposite order. First introduce a diagram (!), then refactor with the helper variables, and then fix the bug. With the refactoring in place *first*, the bugfix should be easier to understand. Then, potentially, generalize the code to larger-than-two multiples of a word, for writes. - The first patch in the series is wrong. In case we need not erase the whole flash block, we will want to write one or two (consecutive) 128-byte "words". That is, 128 bytes, or 256 bytes. That means we need to read the exact same byte counts as well. The *second* patch in the series actually seems to do this, with End =3D (Offset + *NumBytes + BOUNDARY_OF_32_WORDS) & ~BOUNDARY_OF_32= _WORDS; (This *in itself* would *much better* be written as follows: End =3D ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES); but I digress.) However, the first patch still introduces: (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) = + 1 as the byte count for the read. Unfortunately, the "saturation logic" (i.e., OR-ing 0x7F to the exclusive end offset, for "seeking" to the end of the word), and then adding 1, does not implement a correct "align-up" operation. Consider Offset =3D=3D 0 && *NumBytes =3D=3D 256 This circumstance is *valid* for the optimization path (and it is correctly permitted by the top-most check). But the expression introduced by patch#1 produces *384* for it, which is wrong. Similarly, given (for example) Offset =3D=3D 1 && *NumBytes =3D=3D 127 the formula from patch#1 evaluates to 256. The expression does not consider the case when the exclusive end offset of the requested (logical write) is immediately at a word boundary (i.e., a multiple of 128). In that case namely, saturating with the bit-or, and adding 1, is wrong -- because in that case, no additional block should be read at all. So the first patch in the series replaces the *pre-series* bug with a different (less harmful) bug, and then the second patch silently *fixes* the replacement bug. - This is in fact the fundamental bug: the incorrect implementation of the "align-up" operation with "saturate, then add 1". Both the pre-series code, and the code in patch#1, contain this mistake. The only thing that patch#1 changes is the *input*, to which the (incorrect) operation is applied -- namely in patch#1, the *input* changes from "NumBytes" to "exclusive end offset of the logical write, relative to the start of the (double-)word". That input change is in fact good (and *necessary*), but it's not *sufficient*. The operation itself needs to be fixed. Summary: - please rewrite the series in the following order: refactoring, then bugfix, then further armoring (additional sanity checks). - please only ever apply the bit-neg operator on values that are UINT32, UINTN, or UINT64. Otherwise we get sign bit flipping, and that's terrible. (Most people are not even aware of it happening.) - bit-fiddling should be kept to the absolute minimum. This means both a need for helper variables (calculated as early as possible), and usage of macros such as ALIGN_VALUE rather-than open-coded logic. It's possible that the refactoring in patch#2 is effectively impossible to do without fixing the *pre-series* bug at once. That's fine, as long as we point out the bug in the commit message. Importantly, the commit message should provide an actual (Offset, *NumBytes) tuple (an example) where the pre-series expression (*NumBytes | BOUNDARY_OF_32_WORDS) + 1 calculates a bogus byte count for the read. IOW, there are two things to highlight in the commit message: - round-up operation incorrectly implemented, - wrong input provided to the (already incorrect) round-up operation. 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 (#113814): https://edk2.groups.io/g/devel/message/113814 Mute This Topic: https://groups.io/mt/103680930/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-