From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1nqHPZ-0007cR-EL for mharc-grub-devel@gnu.org; Sun, 15 May 2022 12:48:05 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:56424) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nqHPU-0007ao-D7 for grub-devel@gnu.org; Sun, 15 May 2022 12:48:00 -0400 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:43283) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nqHPR-0002BO-Vr for grub-devel@gnu.org; Sun, 15 May 2022 12:48:00 -0400 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.west.internal (Postfix) with ESMTP id 2716B3200A04; Sun, 15 May 2022 12:47:53 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Sun, 15 May 2022 12:47:53 -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=1652633272; x=1652719672; bh=ismONS5QQN ZWW9/PiCIZe3M4K84+nVg16maUh5c/fjc=; b=i7yB3CCddZZT2aRIdrHarFRMon eA+3DDu82CLdZX1mjkWHDtL8Ko4HD0w7RffSalUfUxRTWOiwb1izo6yfxUjvy5Bv KsQ6jMjoBOxtbRXnijl7j0K3JRuP50TYZKo3gi4DT+SfJmLAmOiRYTvgvGANVcQD SOgLCXJNYvTDfax1mjlEyXhBzI5oqHOo5vgbCgg/9y9aHKicgSElhj1U5JEmg6DI Qp0d4yrfojfxRDizRLK6ZOCzMuHtcn+Bl8Y1uHU4VhjM/n+BPQuKT6C9Tm2fPfBp aDjkaOBpVfYjms4TXHuYik0FTpYDQPJNjR6pBHJCGKeYBJEboRIQvS+D/N+A== 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=1652633272; x= 1652719672; bh=ismONS5QQNZWW9/PiCIZe3M4K84+nVg16maUh5c/fjc=; b=K fGDxbDw8elT9hzU2w3Z7LPDGHFv0bk3UraAvEPt7QS/hsTNWsnJf5GS8FbX8/A8Z YHx0nH+HNgfimx1nXSJLiOv3j6BEsubOpHhVGb69POmU5/L8lTHb3r9FSQvKggIV FtNRY2vrJYSVVCdomNGHwfmPinwZQIVC2CnG/UZXeADvDdbEZ8RMsdUdC9xU/0JL s15Up82k5GehdBjp9kHh17EEy1rcuNHxTePdmTQSCEaSZnIm3/Ao1wUp5sCPUff4 4hES2uQWt0UgrKnlZsS6U2+q6dl9X4bes4xCRo9BaUbKkzyoxiqUjJ8Kgov4z7nM as+XRfUesaxVXHtPGdWuA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrheefgddutdegucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvfevuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvg hrnhepueektdevtdffveeljeetgfehheeigeekleduvdeffeeghefgledttdehjeelffet necuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhsse hpkhhsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 15 May 2022 12:47:51 -0400 (EDT) Received: from localhost (xps [10.192.0.12]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 6ef7da93 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 15 May 2022 16:47:48 +0000 (UTC) Date: Sun, 15 May 2022 18:47:47 +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: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="KQuOzPD2WLy3XxFk" Content-Disposition: inline In-Reply-To: Received-SPF: pass client-ip=64.147.123.19; envelope-from=ps@pks.im; helo=wout3-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, 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: Sun, 15 May 2022 16:48:00 -0000 --KQuOzPD2WLy3XxFk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, May 10, 2022 at 11:53:08PM -0500, Glenn Washburn wrote: > Add a --header (short -H) option to cryptomount which takes a file argume= nt. > Using the improved read hook, setup a read hook on the source device which > will read from the given header file during the scan and recovery cryptod= isk > backend functions. This makes supporting detached headers transparent to = the > backend, so long as the attached header is located at the start of the > source disk. This is not the case for GELI volumes, which have the header= at > the end of the volume. So GELI will return an error if a detached header = is > specified. >=20 > Signed-off-by: Glenn Washburn > --- > grub-core/disk/cryptodisk.c | 68 ++++++++++++++++++++++++++++++++++++- > grub-core/disk/geli.c | 4 +++ > include/grub/cryptodisk.h | 2 ++ > include/grub/file.h | 2 ++ > 4 files changed, 75 insertions(+), 1 deletion(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 19af4fa49..4307b723b 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -45,9 +45,18 @@ static const struct grub_arg_option options[] =3D > {"key-file", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING}, > {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYP= E_INT}, > {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TY= PE_INT}, > + {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING}, > {0, 0, 0, 0, 0, 0} > }; > =20 > +struct cryptodisk_read_hook_ctx > +{ > + grub_disk_read_hook_t prev_read_hook; > + void *prev_read_hook_data; > + grub_file_t hdr_file; > +}; > +typedef struct cryptodisk_read_hook_ctx *cryptodisk_read_hook_ctx_t; > + > /* Our irreducible polynom is x^128+x^7+x^2+x+1. Lowest byte of it is: = */ > #define GF_POLYNOM 0x87 > static inline int GF_PER_SECTOR (const struct grub_cryptodisk *dev) > @@ -993,6 +1002,31 @@ cryptodisk_close (grub_cryptodisk_t dev) > grub_free (dev); > } > =20 > +static grub_err_t > +cryptodisk_read_hook (grub_disk_addr_t sector, unsigned offset, > + unsigned length, char *buf, void *data) > +{ > + grub_err_t ret =3D GRUB_ERR_NONE; > + cryptodisk_read_hook_ctx_t ctx =3D data; > + > + if (ctx->hdr_file =3D=3D NULL) > + return GRUB_ERR_NONE; If you intend to keep support for `prev_read_hook`, then this case should likely also execute it, shouldn't it? Furthermore, should we return an error in case `hdr_file =3D=3D NULL`? The = only function that sets this up sets a header file, and it doesn't make a lot of sense to have the hook set up without one. > + if (grub_file_seek (ctx->hdr_file, > + (sector * GRUB_DISK_SECTOR_SIZE) + offset) > + =3D=3D (grub_off_t) -1) > + return grub_errno; > + > + if (grub_file_read (ctx->hdr_file, buf, length) !=3D (grub_ssize_t) le= ngth) > + return grub_errno; > + > + if (ctx->prev_read_hook !=3D NULL) > + ret =3D ctx->prev_read_hook(sector, offset, length, buf, > + ctx->prev_read_hook_data); > + > + return ret; > +} > + > static grub_cryptodisk_t > grub_cryptodisk_scan_device_real (const char *name, > grub_disk_t source, > @@ -1001,6 +1035,7 @@ grub_cryptodisk_scan_device_real (const char *name, > grub_err_t ret =3D GRUB_ERR_NONE; > grub_cryptodisk_t dev; > grub_cryptodisk_dev_t cr; > + struct cryptodisk_read_hook_ctx read_hook_data =3D {0}; > int askpass =3D 0; > char *part =3D NULL; > =20 > @@ -1009,6 +1044,20 @@ grub_cryptodisk_scan_device_real (const char *name, > if (dev) > return dev; > =20 > + if (cargs->hdr_file !=3D NULL) > + { > + read_hook_data.hdr_file =3D cargs->hdr_file; > + > + if (source->read_hook !=3D NULL) > + { > + read_hook_data.prev_read_hook =3D source->read_hook; > + read_hook_data.prev_read_hook_data =3D source->read_hook_data; > + } This part wouldn't need to be conditional. If it was `NULL` before, then the `prev_read_hook` would be `NULL` anyway even without this condition, rendering it essentially pointless. > + 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; > + } So let me check whether I get this right. We're iterating through all crypt= odev devices of the current source disk. In case we are told to use a detached h= eader 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 definitel= y use some comments to explain what the idea behind this is to clarify it a bit f= or future readers. It took me some thinking, but ultimately this does seem to do the right thi= ng. And as you said, it's nice in that the actual backends don't need any chang= es at all. It seems to me like we're not unsetting the hook on the source disk after t= his function return though. We do conditionally restore the previous read hook,= but in case there was none we don't do anything. It's likely not a good idea to= leak the hook to outside callers given that the disk will now essentially be bac= ked by the file. Patrick > if (askpass) > { > cargs->key_len =3D 0; > @@ -1254,6 +1308,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, = int argc, char **args) > return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file"))); > } > =20 > + if (state[7].set) /* header */ > + { > + if (state[0].set) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > + N_("cannot use UUID lookup with detached header")); > + > + cargs.hdr_file =3D grub_file_open (state[7].arg, > + GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER); > + if (cargs.hdr_file =3D=3D NULL) > + return grub_errno; > + } > + > if (state[0].set) /* uuid */ > { > int found_uuid; > @@ -1467,7 +1533,7 @@ GRUB_MOD_INIT (cryptodisk) > grub_disk_dev_register (&grub_cryptodisk_dev); > cmd =3D grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0, > N_("[ [-p password] | [-k keyfile" > - " [-O keyoffset] [-S keysize] ] ]" > + " [-O keyoffset] [-S keysize] ] ] [-H file]" > " "), > N_("Mount a crypto device."), options); > grub_procfs_register ("luks_script", &luks_script); > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index 91eb10122..b3c9bbd80 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -252,6 +252,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t= cargs) > grub_disk_addr_t sector; > grub_err_t err; > =20 > + /* Detached headers are not implemented yet */ > + if (cargs->hdr_file !=3D NULL) > + return NULL; > + > if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH) > return NULL; > =20 > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > index 467065f00..d94df68b6 100644 > --- a/include/grub/cryptodisk.h > +++ b/include/grub/cryptodisk.h > @@ -20,6 +20,7 @@ > #define GRUB_CRYPTODISK_HEADER 1 > =20 > #include > +#include > #include > #include > #ifdef GRUB_UTIL > @@ -79,6 +80,7 @@ struct grub_cryptomount_args > grub_uint8_t *key_data; > /* recover_key: Length of key_data */ > grub_size_t key_len; > + grub_file_t hdr_file; > }; > typedef struct grub_cryptomount_args *grub_cryptomount_args_t; > =20 > diff --git a/include/grub/file.h b/include/grub/file.h > index d53ee8edd..43b47bff5 100644 > --- a/include/grub/file.h > +++ b/include/grub/file.h > @@ -92,6 +92,8 @@ enum grub_file_type > GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY, > /* File holding the encryption key */ > GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY, > + /* File holding the encryption metadata header */ > + GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER, > /* File we open n grub-fstest. */ > GRUB_FILE_TYPE_FSTEST, > /* File we open n grub-mount. */ > --=20 > 2.34.1 >=20 --KQuOzPD2WLy3XxFk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmKBLrIACgkQVbJhu7ck PpQUGw/+NfVE+r23DJPYB7l+2ct4kKGfLsKEG79f91fb6KsHMXQmRZihCBWvjrDr 1WCT6UgH0mxpei8hzeA0VvJuWJVDXzkRzP6ju9T+m0nveVRE+Y5Bogu7wBboy9uX iiBm0BCLHS6CqKaxtRS+KJve1tfHDk56VeO4TXwTr7ggJXjRF5U+WuFZVic7143h FC7JKQ4XAPMRhe2cBF6S8HZd+pg9lW/V2s2gGn5F1PsSNWkvu+n0Y0an5Y2HFmn3 kn8VMT0awdMGEc9vWfjiH8AP0YcsaTdawBnLQHrSmQk9xCJyQ/47G9d53et8hB4R 5G3yoLSLySZH7sw+Dyfdq5/C+Dxej8/GAFZsjzRcZQJ0XJtsDhXXyVSIqyrhFqLO CMw7mG+Cgk6TlnEuUGk7l8UzXd8XOkGYcpMGRohciNvD0JGPXyx1yVg5KxMJOH4O Kg4iB1xiS+mgEuhxpE44igqY9qrTQybsv2rV2lgZ2pEfm/mRR7kIQsMkEHkOyng6 7b94wodv+TKAfzPyz61RFcJ3vRBsmsFkojePJq/hG46bCZTwvTrPwvSUIElr45bZ 94fS9MB+9+bWMABdQjesti1JKyKekZCCTo+B0iqruv9MLNqg+UFnQPW5NHN6LIfW Z16Oj23rcB+wSTnOgBjm185zu38e9II8cPKoDdlChFzrsF0KmWk= =ZDjB -----END PGP SIGNATURE----- --KQuOzPD2WLy3XxFk--