From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1j8Jva-0002xZ-L2 for mharc-grub-devel@gnu.org; Sun, 01 Mar 2020 03:26:22 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:42164) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1j8JvW-0002xO-U5 for grub-devel@gnu.org; Sun, 01 Mar 2020 03:26:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1j8JvV-0003P8-8m for grub-devel@gnu.org; Sun, 01 Mar 2020 03:26:18 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:52097) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1j8JvU-0003Ok-RH for grub-devel@gnu.org; Sun, 01 Mar 2020 03:26:17 -0500 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 2FB942248C; Sun, 1 Mar 2020 03:26:14 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Sun, 01 Mar 2020 03:26:14 -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=fm3; bh=SHD6tDI5PM9eoFu/pXLJzssUDyd W199HTA7c1//4dtM=; b=cwLV/vYJZ3pfAY572NbKSC2jKccF7lroCQVU2HiLgPI 9xlsd01yXE3rUQWC+7SYVieGz9DiRT15aIEswvtLxCOJn06ETUrqB2muQvdLkAe5 fYSj2LFQ3grYW8bnmxo47PRkvoi9TOqkTbBtg6Kh10Wt/JaYzAr7wh2vE83sGcBs TRh9KPAomMqfYiQqKHN3MnJRoT/9qiCrnuMy0O4PMLsnjqZ52bQhb479kC/RjzW9 tOKsiRV1FCAXCimHmyhuc/9/6xEtOqgfHAF2ewRoS2gCjBVaXQ/VM+McXx2R7FEQ 64oCWAWer4jCy5hyjk8yWVej5JIDbhb0IV1YD/ArSzQ== 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=fm2; bh=SHD6tD I5PM9eoFu/pXLJzssUDydW199HTA7c1//4dtM=; b=KiaH8+gVroA6DZpIEXdXXm niCTMdO2qrHw0T7ClV5DoDSEewouk4dPRfQVF2dCd55qHQ9LgVTiAJikZLgPOMbX yyuP1kF7OxN9xwtHhjyPO8Yl4z3ayI3m9wpwrfxpEU7lDfcGd99lSsPmCkc+s1qe Me8zikZItpbOMiw5NVjf9ksccJRNIn6FWwiC3YIv8P49tN78Myt4IvsfdMw7tYHD LMuQ+UisUlnve1YcEsXTkTjbqYDRfIMwsYpxSgprhXQrasuM1FJwFbD8gn5KF5bC Ceen4JPi0Vj0/89uXiPPGQCxBYqXGosxU4z2wcdr6QtgJwCMPDe/8RrvT0alq9iA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedruddtuddguddujecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecukfhppeejjedrud efrddvfeekrddunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhf rhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Received: from vm-mail (x4d0dee01.dyn.telefonica.de [77.13.238.1]) by mail.messagingengine.com (Postfix) with ESMTPA id 400A030611F2; Sun, 1 Mar 2020 03:26:12 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail (OpenSMTPD) with ESMTPSA id c2838adf (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 1 Mar 2020 08:26:10 +0000 (UTC) Date: Sun, 1 Mar 2020 09:26:09 +0100 From: Patrick Steinhardt To: The development of GNU GRUB Cc: John Lane , Denis 'GNUtoo' Carikli Subject: Re: [PATCH 1/2] Cryptomount support LUKS detached header Message-ID: <20200301082609.GA9034@ncase> References: <20200221210349.22490-1-GNUtoo@cyberdimension.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mYCpIKhGyMATD0i+" Content-Disposition: inline In-Reply-To: <20200221210349.22490-1-GNUtoo@cyberdimension.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.111.4.28 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 01 Mar 2020 08:26:20 -0000 --mYCpIKhGyMATD0i+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 21, 2020 at 10:03:48PM +0100, Denis 'GNUtoo' Carikli wrote: > From: John Lane >=20 > Signed-off-by: John Lane > GNUtoo@cyberdimension.org: rebase > Signed-off-by: Denis 'GNUtoo' Carikli We usually prefix commit subjects by the respective subsystem they're touching. For example something like "cryptodisk: add support for detached headers". It would also be nice to have the commit message explain what you're doing in this commit in the future. > --- > grub-core/disk/cryptodisk.c | 22 ++++++++++++++---- > grub-core/disk/geli.c | 7 ++++-- > grub-core/disk/luks.c | 45 ++++++++++++++++++++++++++++++------- > include/grub/cryptodisk.h | 5 +++-- > include/grub/file.h | 2 ++ > 5 files changed, 65 insertions(+), 16 deletions(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 1897acc4b..6d4befc6f 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =3D > /* TRANSLATORS: It's still restricted to cryptodisks only. */ > {"all", 'a', 0, N_("Mount all."), 0, 0}, > {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0= }, > + {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STR= ING}, > {0, 0, 0, 0, 0, 0} > }; > =20 > @@ -970,6 +971,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk) > =20 > static int check_boot, have_it; > static char *search_uuid; > +static grub_file_t hdr; > =20 > static void > cryptodisk_close (grub_cryptodisk_t dev) > @@ -994,13 +996,13 @@ grub_cryptodisk_scan_device_real (const char *name,= grub_disk_t source) > =20 > FOR_CRYPTODISK_DEVS (cr) > { > - dev =3D cr->scan (source, search_uuid, check_boot); > + dev =3D cr->scan (source, search_uuid, check_boot, hdr); > if (grub_errno) > return grub_errno; > if (!dev) > continue; > =20 > - err =3D cr->recover_key (source, dev); > + err =3D cr->recover_key (source, dev, hdr); > if (err) > { > cryptodisk_close (dev); > @@ -1041,7 +1043,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev,= const char *cheat) > =20 > FOR_CRYPTODISK_DEVS (cr) > { > - dev =3D cr->scan (source, search_uuid, check_boot); > + dev =3D cr->scan (source, search_uuid, check_boot,0); > if (grub_errno) > return grub_errno; > if (!dev) > @@ -1095,6 +1097,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, = int argc, char **args) > if (argc < 1 && !state[1].set && !state[2].set) > return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required"); > =20 > + if (state[3].set) /* LUKS detached header */ > + { > + if (state[0].set) /* Cannot use UUID lookup with detached header */ > + return GRUB_ERR_BAD_ARGUMENT; We should return a proper error to the user so that he knows that both options are incompatible. E.g. return grub_error (GRUB_ERR_BAD_ARGUMENT, "Cannot use UUID lookup with detached header"); That'd also make the comment obsolete. > + > + hdr =3D grub_file_open (state[3].arg, GRUB_FILE_TYPE_LUKS_DETACHED= _HEADER); > + if (!hdr) > + return grub_errno; > + } > + else > + hdr =3D NULL; > + > have_it =3D 0; > if (state[0].set) > { > @@ -1302,7 +1316,7 @@ GRUB_MOD_INIT (cryptodisk) > { > grub_disk_dev_register (&grub_cryptodisk_dev); > cmd =3D grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0, > - N_("SOURCE|-u UUID|-a|-b"), > + N_("SOURCE|-u UUID|-a|-b|-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 e9d23299a..f4394eb42 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -52,6 +52,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev) > =20 > static grub_cryptodisk_t > configure_ciphers (grub_disk_t disk, const char *check_uuid, > - int boot_only) > + int boot_only, > + grub_file_t hdr __attribute__ ((unused)) ) > { > grub_cryptodisk_t newdev; > struct grub_geli_phdr header; > @@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char *chec= k_uuid, > } > =20 > static grub_err_t > -recover_key (grub_disk_t source, grub_cryptodisk_t dev) > +recover_key (grub_disk_t source, grub_cryptodisk_t dev, > + grub_file_t hdr __attribute__ ((unused)) ) > { > grub_size_t keysize; > grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN]; You've probably started working on an oldish commit, as we've gained LUKS2 support in the meantime. Means that you'll also have to change function signatures for LUKS2 as it will otherwise fail to compile. > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 410cd6f84..950e89237 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, = grub_uint8_t * src, > =20 > static grub_cryptodisk_t > configure_ciphers (grub_disk_t disk, const char *check_uuid, > - int check_boot) > + int check_boot, grub_file_t hdr) > { > grub_cryptodisk_t newdev; > const char *iptr; > @@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char *chec= k_uuid, > char hashspec[sizeof (header.hashSpec) + 1]; > grub_err_t err; > =20 > + err =3D GRUB_ERR_NONE; > + err could just be initialized directly with its declaration. > if (check_boot) > return NULL; > =20 > /* Read the LUKS header. */ > - err =3D grub_disk_read (disk, 0, 0, sizeof (header), &header); > + if (hdr) > + { > + grub_file_seek (hdr, 0); > + if (grub_file_read (hdr, &header, sizeof (header)) !=3D sizeof (head= er)) > + err =3D GRUB_ERR_READ_ERROR; > + } You should check the return value of `grub_file_seek()`. I also feel like we should not clobber `err` with our own value, but instead we should probably return error codes of `grub_file_seek()` and `grub_file_read()` directly. > + else > + err =3D grub_disk_read (disk, 0, 0, sizeof (header), &header); > + > if (err) > { > if (err =3D=3D GRUB_ERR_OUT_OF_RANGE) > @@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char *ch= eck_uuid, > } > =20 > COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >=3D sizeof (uuid)); > + > return newdev; Needless code churn :) > } > =20 > static grub_err_t > luks_recover_key (grub_disk_t source, > - grub_cryptodisk_t dev) > + grub_cryptodisk_t dev, > + grub_file_t hdr) > { > struct grub_luks_phdr header; > grub_size_t keysize; > @@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source, > grub_err_t err; > grub_size_t max_stripes =3D 1; > char *tmp; > + grub_uint32_t sector; > + > + err =3D GRUB_ERR_NONE; > + > + if (hdr) > + { > + grub_file_seek (hdr, 0); > + if (grub_file_read (hdr, &header, sizeof (header)) !=3D sizeof (head= er)) > + err =3D GRUB_ERR_READ_ERROR; > + } Same as above with regards to error handling. The rest looks fine to me. Patrick --mYCpIKhGyMATD0i+ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl5bcZ8ACgkQEXxntp6r 8SwKqQ//e7A2LGeBLjtUP/5Ft5gOT58IeOxTToc4852NjWyhhgGegp4pBXzPo82x Ngt0uXLI+IijIYg2ImyMss/XfrdA3x5TCTuZDO/RzeMNNLEbSDW6ZZxlXZlM4Idv o61TxfYWwC0G1n9WpNH2+P1iguPg6Pc9UfF9ECF5DWmojWj9Su3vYUXgZCACcDlM cXW6xqP3QP5gx+gasn44sjC8OCNu9tz4MTJ+yov9mK/uKX/SuBMjlbl14AHREKi2 5OBa+BWyTmUXkJfpgEvVUAjkqWHjIlxeNUm6H+IWMdAY/V0c5OMxCCXDfS0Y+dK/ rpNWVA7BpFfH4OcA3h+lSIqN/f8cb4iucoxhd33i0RLfJkMpdxgnx70Lnz1dPRFv yH1qwsHo8ik3IHQ18CgKNel7tLc1W+m+VBHrt6sKFRSoggN7+1ALFdyKNtXtZcM5 ETUOjykisM2uiUxlC4nHWwFj8RvJ+sX7kjmxGZW+uGcGFSJlwMVKPOQp4CllKOQV 0ynhXny5++Y2xgpXUP04XSGx7VP1tHTDflopsAq5Z3bKmUddm68XUceqq2p+TZiT icL5DjMntTybcCqW3aogoVRrBy/jGGzKf4SJcKaRUrWOyugNFQLrOcqibq/0XP4c YM87RFbq3RrrEiIzJAT6sRbsuL5tv+uw+XBOFbFO9zMPl02Ybhs= =AqJK -----END PGP SIGNATURE----- --mYCpIKhGyMATD0i+--