From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mmCGb-0000mD-Ak for mharc-grub-devel@gnu.org; Sun, 14 Nov 2021 04:57:41 -0500 Received: from eggs.gnu.org ([209.51.188.92]:41050) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mmCGa-0000m4-89 for grub-devel@gnu.org; Sun, 14 Nov 2021 04:57:40 -0500 Received: from wout4-smtp.messagingengine.com ([64.147.123.20]:39205) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mmCGY-0004kP-CN for grub-devel@gnu.org; Sun, 14 Nov 2021 04:57:39 -0500 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id 1BE143200A1B; Sun, 14 Nov 2021 04:57:36 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Sun, 14 Nov 2021 04:57:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=fPjU1oJGxQxt9VVr5mliVm8hHVi zYZYflRkaarmZ2MY=; b=FQEFdc01SQ0Rmp2DmPs+0BwPKyyCRA1EQPH1vPcwH8s 5HNRP0PaWVXB30YzLEFTkbMq+dJGSnRre2D/haM38dMgESI54ow6mm58LuGldwDr D+od/deqOE7bVEzQvcg7oKsRLFg9Q3qs87rqJY7nnm54yptkZgMWuDROMwc06k0Z V6SoNBQBHjARJCzcKPykfgxcVC/ndeGewwytrOsLGDvJLmQonbWgBcIB1D6JpnUY pviwEKoa0CqkBTX1QSLaH0dxUFPxgU6Ja9KkG+GqCOAZVjpmrD0HibWDhIoGluNK EYNSVlZHadEjsZMHLqssOUZskzUXa89S27xM38QzaWQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=fPjU1o JGxQxt9VVr5mliVm8hHVizYZYflRkaarmZ2MY=; b=QuwPotiG0098W39vAT1nZJ kuX8G2xhxGHF0xqVEhiiRTjdXZ9h2pSV+z508TITA6mJ2RjAlxwpjX2+IK1pF6f8 QS9bvRHQEwWcTzl6ATjewjlFnt2JCTFXnVF7S8RvAtZ8Csq4CchRktQB77nqKm3/ ZsZW3TnXWnyqPYBUrBlD0MtGlNEQ1rxhffo2uZkvPKXvLheB9fiCioZwJjCXKKem Z7SCsqws2L46uaIkLmjpmHE7tFmqsCVpoxw8FsdvkT+P+aS4YKShkdHx8KnfgqXc kLPmXYET35roREgFZhq3n4L5jUc9TMsu6uKN0H94YGsa1IU7TDjypJZXkylOVkdw == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvuddrvdejgddtlecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujgesghdtroertddtvdenucfhrhhomheprfgrthhrihgt khcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvghrnh epvdeffedvhfelheehleeghedtveeiieekffduvedvudetfeffjeffjeelleefvefhnecu vehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkh hsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 14 Nov 2021 04:57:34 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 9fdb989e (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 14 Nov 2021 11:40:49 +0000 (UTC) Date: Sun, 14 Nov 2021 10:57:09 +0100 From: Patrick Steinhardt To: Glenn Washburn Cc: Daniel Kiper , grub-devel@gnu.org, Denis 'GNUtoo' Carikli , James Bottomley Subject: Re: [PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto modules Message-ID: References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="AP1wMae7JrURlkAd" Content-Disposition: inline In-Reply-To: Received-SPF: pass client-ip=64.147.123.20; envelope-from=ps@pks.im; helo=wout4-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_H2=-0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 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, 14 Nov 2021 09:57:40 -0000 --AP1wMae7JrURlkAd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 12, 2021 at 06:26:25PM -0500, Glenn Washburn wrote: > Updates since v2: > * Add space after function name in function calls > * Add comments for members of struct grub_cryptomount_args > * Correct commit message for patch #4 >=20 > --- > This patch series refactors the way cryptomount passes data to the crypto > modules. Currently, the method has been by global variable and function c= all > argument, neither of which are ideal. This method passes data via a > grub_cryptomount_args struct, which can be added to over time as opposed = to > continually adding arguments to the cryptodisk recover_key (as is being > proposed in the keyfile and detached header patches). >=20 > The infrastructure is implemented in patch #1 along with adding a new -p > parameter to cryptomount partly as an example to show how a password woul= d be > passed to the crypto module backends. The backends do nothing with this d= ata > in this patch, but print a message saying that sending a password is > unimplemented. >=20 > Patch #2 takes advantage of this new data passing mechanism to refactor t= he > essentially duplicated code in each crypto backend module for inputting t= he > password and puts that functionality in the cryptodisk code. Conceptually, > the crypto backends should not be getting user input anyway. >=20 > Patch #3 gets rid of some long time globals in cryptodisk, moving them > into the passed struct. >=20 > Patch #4 removes the found_uuid flag from the cargs struct, which is not > needed because the same information can be obtained from the return value > of grub_device_iterate. >=20 > My intention is for this patch series to lay the foundation for an improv= ed > patch series providing detached header and keyfile support (I already have > the series updated and ready to send once this is accepted). I also belie= ve > tha this will somewhat simplify the patch series by James Bottomley in > passing secrets to the crypto backends. >=20 > Glenn A single question for patch 3/4, but all the others look good to me. Feel free to add "Reviewed-by: Patrick Steinhardt " to those. Patrick > Glenn Washburn (4): > cryptodisk: Add infrastructure to pass data from cryptomount to > cryptodisk modules > cryptodisk: Refactor password input out of crypto dev modules into > cryptodisk > cryptodisk: Move global variables into grub_cryptomount_args struct > cryptodisk: Remove unneeded found_uuid from cryptomount args >=20 > grub-core/disk/cryptodisk.c | 108 ++++++++++++++++++++++++------------ > grub-core/disk/geli.c | 35 ++++-------- > grub-core/disk/luks.c | 37 ++++-------- > grub-core/disk/luks2.c | 33 ++++------- > include/grub/cryptodisk.h | 19 ++++++- > 5 files changed, 120 insertions(+), 112 deletions(-) >=20 > Range-diff against v2: > 1: 475bf7e9e ! 1: 83112518f cryptodisk: Add infrastructure to pass data= from cryptomount to cryptodisk modules > @@ grub-core/disk/cryptodisk.c: static grub_err_t > + if (state[3].set) /* password */ > + { > + cargs.key_data =3D (grub_uint8_t *) state[3].arg; > -+ cargs.key_len =3D grub_strlen(state[3].arg); > ++ cargs.key_len =3D grub_strlen (state[3].arg); > + } > + > have_it =3D 0; > 2: a0c0694fc ! 2: 65a18c5e8 cryptodisk: Refactor password input out of = crypto dev modules into cryptodisk > @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (con= st char *name, > + ret =3D grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not sup= plied"); > + goto error; > + } > -+ cargs->key_len =3D grub_strlen((char *) cargs->key_data); > ++ cargs->key_len =3D grub_strlen ((char *) cargs->key_data); > + } > + > + ret =3D cr->recover_key (source, dev, cargs); > @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (con= st char *name, > + if (askpass) > + { > + cargs->key_len =3D 0; > -+ grub_free(cargs->key_data); > ++ grub_free (cargs->key_data); > + } > + return ret; > } > 3: 419f114d8 ! 3: fed38b3dc cryptodisk: Move global variables into grub= _cryptomount_args struct > @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const ch= ar *name, > =20 > static grub_err_t > @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_co= ntext_t ctxt, int argc, char **args) > - cargs.key_len =3D grub_strlen(state[3].arg); > + cargs.key_len =3D grub_strlen (state[3].arg); > } > =20 > - have_it =3D 0; > @@ include/grub/cryptodisk.h: typedef gcry_err_code_t > =20 > struct grub_cryptomount_args > { > ++ /* scan: Flag to indicate that only bootable volumes should be de= crypted */ > + grub_uint32_t check_boot : 1; > + grub_uint32_t found_uuid : 1; > ++ /* scan: Only volumes matching this UUID should be decrpyted */ > + char *search_uuid; > ++ /* recover_key: Key data used to decrypt voume */ > grub_uint8_t *key_data; > ++ /* recover_key: Length of key_data */ > grub_size_t key_len; > }; > + typedef struct grub_cryptomount_args *grub_cryptomount_args_t; > @@ include/grub/cryptodisk.h: struct grub_cryptodisk_dev > struct grub_cryptodisk_dev *next; > struct grub_cryptodisk_dev **prev; > 4: e6e1e8bc3 ! 4: 854709929 cryptodisk: Remove unneeded found_uuid from= cryptomount args > @@ Commit message > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_s= can_device > will only return 1 if a search_uuid has been specified and a cry= ptodisk was > successfully setup by a crypto-backend. So the return value of > - grub_cryptodisk_scan_device is equivalent to found_uuid. > + grub_cryptodisk_scan_device is almost equivalent to found_uuid, = with the > + exception of the case where a mount is requested or an already o= pened > + crypto device. > + > + With this change grub_device_iterate will return 1 when > + grub_cryptodisk_scan_device when a crypto device is successfully= decrypted > + or when the source device has already been successfully opened. = Prior to > + this change, trying to mount an already successfully opened devi= ce would > + trigger an error with the message "no such cryptodisk found", wh= ich is at > + best misleading. The mount should silently succeed in this case,= which is > + what happens with this patch. > =20 > ## grub-core/disk/cryptodisk.c ## > @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (co= nst char *name, > @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_con= text_t ctxt, i > } > =20 > ## include/grub/cryptodisk.h ## > -@@ include/grub/cryptodisk.h: typedef gcry_err_code_t > - struct grub_cryptomount_args > +@@ include/grub/cryptodisk.h: struct grub_cryptomount_args > { > + /* scan: Flag to indicate that only bootable volumes should be de= crypted */ > grub_uint32_t check_boot : 1; > - grub_uint32_t found_uuid : 1; > + /* scan: Only volumes matching this UUID should be decrpyted */ > char *search_uuid; > - grub_uint8_t *key_data; > - grub_size_t key_len; > + /* recover_key: Key data used to decrypt voume */ > --=20 > 2.27.0 >=20 --AP1wMae7JrURlkAd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmGQ3XQACgkQVbJhu7ck PpR3rxAAlLItgUez2hXr2sVNvI65ZHP8krogth2FJu3ahDFSAV/RScVNvR48qget Ipy7Y92Ldm2Vq2SPY+84aCItHNu3rU4Fzyh3rlMQ0ul5z80jkwcyCp8Udjw4pHRb iq98rRqboKsot0owNscEDj+oAWOIqBCsGpg+xK9XSh7hsRZmPNak1ArCS1+NpBb6 3ey7CIDJceKrhob+s0NDXG9gL+ludTlgLBIPvMF3zrXZR2o/cvrXemc9Y4Wp6lul hUtLN8XJyb9hkzwyRqv+i/nN20S6cDh5jSqKs/oQySiOdYKgWOXENiY0fi1Ar4Jc 3RvrpfgF66ySr5BI62ZqLmhWJXmD06FbH9UJp5zXS+qXwQjTVome80sm5s7ejSMx Az7KWh69NC7rtkobH5tuKnNk9tCBwDIm8i6xc8nFt8YMDAEVT4a92x3j+Lq/W2DP kpRa4xLhMW4TOanvHyR5SZ4WDOcdz7lFOjxCLWotq111VtHhsHcC+DL0gsHMsUgP tuloGJ0Qf2GkJfjOB8alZ2BVC3BsY4jYX3tKk0vD8Q6qxXda2Hl0txKPiWldelPE CK0mn26L57yKRxklUvp6nfsH85SBfXcQ8CqpLX1BMq7mtFwicQq5A9OIR51vmIHt FdXREiuVTnME7qMfV49X1MDoZoHs7e+47SCl80k0bByLV92ZYp0= =2r+p -----END PGP SIGNATURE----- --AP1wMae7JrURlkAd--