From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1nqcoT-0004at-K0 for mharc-grub-devel@gnu.org; Mon, 16 May 2022 11:39:13 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:35966) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nqcoS-0004VY-3X for grub-devel@gnu.org; Mon, 16 May 2022 11:39:12 -0400 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:33385) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nqcoN-0008Ka-Qw for grub-devel@gnu.org; Mon, 16 May 2022 11:39:11 -0400 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id E556A5C01FE; Mon, 16 May 2022 11:39:06 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Mon, 16 May 2022 11:39:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; t=1652715546; x=1652801946; bh=pegfqaLq3N obs1iyVicy/cp2OqgariurHK8OOT4N1Ao=; b=qyaA6GBqCnddqGZ497jScze3N3 wkLBG7TVaSdZi+0V8Iv7q6EjCmDcriwoWvMXNA9Xe6CrVRJR74t/xetKCnXhQLx7 UoRHG68eTQIRSr0OXAA2oukJJZmF/kBP9jx2Bvd/4wSqGZsKtc+XAoD/Gvlf42lO fT9IUQtE7gqg1AHlFCogESpKMetNyeVfPL94pOEogQKTSv+QvpAjDEK0WtUiN9R7 5vuq/HZ8Jw3XGQ6ulfBspBFSEEDrE9yEbnMgbXyfurduBnYTwmAyPGMqbEwykEkQ b8Cb2NzCnFSQkF+t+spFtdVKXYWsrhfxoaINrKBKhLe66GNHJlt1OPNJ65ag== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1652715546; x= 1652801946; bh=pegfqaLq3Nobs1iyVicy/cp2OqgariurHK8OOT4N1Ao=; b=E nGcsrH+SfOT9zKRUcYzQS+0WrL/vZP+/XApnRcwf9gX2zgApXLzzqwncFE1H8X5g pNHlqFqObW0CwqFzwixbyFw3zIVAMo1sB/3i/1wa9Bwi+1/bm9gZQ7DKeGLPp74G 4RuF9z8YSLzhqTwaaaUKf6pwWTuGvQvsSy3MT+h9qs+LKfa7GrMZpZ0pinyDqBAY a31yswZ2JjfgG/KcGODAu/+T6vMthDKuBQ/Hq8oygKjtB+VBoEiQ4MwfM6Z9lSTt j11bUUmxmeObzWs2XGAlqFMnbdNbmHwkMVkU3WBgw4dOc2efRldF90/6VBHW8yGf ven9NPaky9CMLZPa0ZXUQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrheehgdeklecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeeukedtvedtffevleejtefgheehieegkeeluddvfeefgeehgfeltddtheejleffteen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 16 May 2022 11:39:05 -0400 (EDT) Received: from localhost (xps [10.192.0.12]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id cfc6761c (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Mon, 16 May 2022 15:39:02 +0000 (UTC) Date: Mon, 16 May 2022 17:39:01 +0200 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper , Denis 'GNUtoo' Carikli , John Lane Subject: Re: [PATCH 2/3] cryptodisk: Add --header option to cryptomount to support detached headers Message-ID: References: <20220516092639.01aa7ce1@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="9uMr63iEa1hlhPNu" Content-Disposition: inline In-Reply-To: <20220516092639.01aa7ce1@crass-HP-ZBook-15-G2> Received-SPF: pass client-ip=66.111.4.26; envelope-from=ps@pks.im; helo=out2-smtp.messagingengine.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 May 2022 15:39:12 -0000 --9uMr63iEa1hlhPNu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, May 16, 2022 at 09:26:39AM -0500, Glenn Washburn wrote: > On Sun, 15 May 2022 18:47:47 +0200 > Patrick Steinhardt wrote: >=20 > > On Tue, May 10, 2022 at 11:53:08PM -0500, Glenn Washburn wrote: [snip] > > > + source->read_hook =3D cryptodisk_read_hook; > > > + source->read_hook_data =3D (void *) &read_hook_data; > > > + } > > > + > > > FOR_CRYPTODISK_DEVS (cr) > > > { > > > dev =3D cr->scan (source, cargs); > > > @@ -1058,6 +1107,11 @@ grub_cryptodisk_scan_device_real (const char *= name, > > > dev =3D NULL; > > > =20 > > > cleanup: > > > + if (read_hook_data.prev_read_hook !=3D NULL) > > > + { > > > + source->read_hook =3D read_hook_data.prev_read_hook; > > > + source->read_hook_data =3D read_hook_data.prev_read_hook_data; > > > + } > >=20 > > So let me check whether I get this right. We're iterating through all c= ryptodev > > devices of the current source disk. In case we are told to use a detach= ed header > > we now just set a read callback function that replaces whatever we did = read with > > the contents of the detached header. I think that this code could defin= itely use > > some comments to explain what the idea behind this is to clarify it a b= it for > > future readers. >=20 > Yes, in my words, I'd say: we're iterating through the registered > cryptodisk device backends, checking if a certain source disk can be > opened by that backend (ie scan returns non-NULL). If we've been given > a detached header to use, we set a read hook, which will redirect any > reads on the source disk to a read from the header file at the same > offset as it would've been to the source disk. Its an artifact of the > disk read hook mechanism that the source disk will be read from and > then the read data will be overwritten by the read hook with the read > data from the header file. I'm not sure this need be documented. Do you > think so? It would likely help reading the code when you ain't got the original context of the commit. > I agree some comments would be good. Where would've you liked to see > comments when you were reading this and what do you think would be > helpful? Since I'm removing the prev_read_data code, no need to comment > that. I'm thinking above the cryptodisk_read_hook definition with > something like: "This read hook is used read a detached header file of > a cryptodisk device instead of reading the header from the device. > Currently, it can only be used when the header is located at the start > of the volume, as is the case for LUKS1 and LUKS2, but not GELI." What took me longest to figure out was the lifetime of the hook. I was confused that it wasn't unset after we return, which is fine in case we know that the caller would destroy the disk anyway. But it's not obvious, and for code hygiene I'd in fact change that assumption so that any future changes don't break that assumption. Furthermore, I was confused at first that the hook is invoked multiple times and how that can in fact work. Until I realized that it is fine for the hook to be called as many times as we want to, as long as every single read really only reads in the area where we expect the header to be. I think this loop here would also be the best location to document on a comparatively high level what the hook does, what the basic assumption is, and how the called functions are impacted. If we continue to not reset the hook, then we should also document why so that the reader doesn't have to figure it out by themself. > > It took me some thinking, but ultimately this does seem to do the right= thing. > > And as you said, it's nice in that the actual backends don't need any c= hanges at > > all. > >=20 > > It seems to me like we're not unsetting the hook on the source disk aft= er this > > function return though. We do conditionally restore the previous read h= ook, but > > in case there was none we don't do anything. It's likely not a good ide= a to leak > > the hook to outside callers given that the disk will now essentially be= backed > > by the file. >=20 > Yes, this is an inconsistency. I didn't unset the hook because the > source disk is closed right away in both callers of > grub_cryptodisk_scan_device_real. But then it doesn't make sense to do > the prev_read_hook because the disk has just been opened (if we assume > that opening a disk doesn't set any hooks). So I'm removing that code. >=20 > Glenn Fair enough. As stated above, this is not at all obvious though when only reading this function. So I'd either just unset it so that the reader is satisfied and not left wondering why we don't. Or explain why we don't need to reset it. Personally, I'd lean towards just resetting the hook because it feels tidier to me. Even if the caller changes we wouldn't leak the hook in any way. Patrick --9uMr63iEa1hlhPNu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmKCcBQACgkQVbJhu7ck PpTXNQ//Q+0nb1rhlX4fio08DK9WE5QdiQUyFn4y9CDbgEM/G90ZDz9KJGmklnuZ zOFT4YRKWyl0hZ9OghKkrR33i6SU1f11ywOsgDTsj1ec4HAEXhyPOdGsUuGeWjR8 OmzDIDTE7MV0oGW/+KY6N/xPVp1gZRhz+UhOxDfokYYwS5rOSPAtkCWYofqm+thi x6IvhRNDCy13CvmpxlSJWPpzxSJfuDsfglQB9OBduEnHmG+BwU+IkmgF7ul51YSN 7hI5IEAiWUopycEErAI2vCONwZVTWOop9sOVa+983n6XcmjObiLrCMpwvYpXfIqZ DBDqcbaTmGi3FxkT0cJTosR3Dd6boeU1jTB1I5nBJokyIvl7tq4qhq5pBv0amIv7 qXMU527MuaFizC+QrefAKSBc/c6bPoxcFtFHXU+084Bd2KhGKpFhwW1bQ1Qptexp pzgSSVdTFaVllvXs8O81r++hLFOpFgrP2Is78XgC+nR5/yQxHL4FxsNuNSpmM4PX RIfnhD56X8RFfOLZKRQ53Eyhmw/z7glRgynRTvRBL0yYMmGuTOW31/38LxlRPqrV QERaPWZmUUBv1NJOElUUbeZj6sy8/f3U/W1qytKWocqrH8DZDNaubHFjxl3rTbec sqvCaZzhEgIpvMfwgXa5N2cYjO4Lz4UkNdQE4fsa9OZh+hFDL9E= =Mihb -----END PGP SIGNATURE----- --9uMr63iEa1hlhPNu--