From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kmWgt-0003pW-TQ for mharc-grub-devel@gnu.org; Tue, 08 Dec 2020 01:41:39 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:40404) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kmWgp-0003p7-Ae for grub-devel@gnu.org; Tue, 08 Dec 2020 01:41:37 -0500 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:40303) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kmWgn-00008H-8G for grub-devel@gnu.org; Tue, 08 Dec 2020 01:41:34 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 929BFCC9; Tue, 8 Dec 2020 01:41:31 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 08 Dec 2020 01:41:31 -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=oCPq6JBw+V75gxVyPAe3Krv8Kp1 chQkfFLGxy8D7/s4=; b=LSMvdzs381eemIyqvOsN0Q3eitK64DDR0a/qa83LnUm CbRcHWQ1ahxgllGB6M4GPnGZZ6EPyeogVqMaCdDk7imrNLv5FHLB2bH1ru9395p4 GspCcBh6amZMaOz7993MBn8WvxAIK9QTn9dRQAa2c+E5zc33+XBls1yGleCwBRLT kCopaj0/nOUhVgNbIvKRKY26uZKGDOkbl0h5zVQ5PNxApNwMAyniFDhx2yZtGQk3 iBTk+oMQ6+W3BwSz7HIoo0s2HKnHSkIgZRCEDX9Hzh+y+wXWyf14/7jLiyKSiAf6 ObxNQRqYDuqb8vLi/36GZe1wVq1K9jENS2PJOMo1e/A== 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=oCPq6J Bw+V75gxVyPAe3Krv8Kp1chQkfFLGxy8D7/s4=; b=WhK1B0a3r7P5Av/e6qEa02 T5kQLRED7CXjOxZ1EVOZQ4qCm96vqNslg8Yj3xBrRIXAEh5ohe4mZba4YaW5e2Uh Txf207mSxwq/noDiVHuRZQ9wqNjbGUW5GxQGTIcAZ42KNCSB1vAGln7lEOL4y86C gmcgPubiP0a5lLfkBBevotWPcffikSUUQjqZdMppzxZbBMcHwAv33AwPXGGXgHRK hH5hyg33mJHr6oNvFrlU/AWBgJLpgfwo6k111MPGkruM7nTF94fvpWMS7/FYkM5n gZ/Vf3QwsWvQd4KYzV1V3p2RTLIjuh6OcPpcHFQwMk01Kr45hGua8SOr1MWXoEzw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudejhedgleelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucfkphepjeejrdduledurdeguddrudehieenucevlhhushhtvghrufhiiigvpedtnecurf grrhgrmhepmhgrihhlfhhrohhmpehpshesphhkshdrihhm X-ME-Proxy: Received: from vm-mail (x4dbf299c.dyn.telefonica.de [77.191.41.156]) by mail.messagingengine.com (Postfix) with ESMTPA id B2D6A24005D; Tue, 8 Dec 2020 01:41:29 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail (OpenSMTPD) with ESMTPSA id 5645f9df (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 8 Dec 2020 06:41:26 +0000 (UTC) Date: Tue, 8 Dec 2020 07:41:25 +0100 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper Subject: Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk Message-ID: References: <20201207220401.78adad64@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Din0jlarDx3XYfSq" Content-Disposition: inline In-Reply-To: <20201207220401.78adad64@crass-HP-ZBook-15-G2> 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, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=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.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Dec 2020 06:41:37 -0000 --Din0jlarDx3XYfSq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 07, 2020 at 10:04:01PM -0600, Glenn Washburn wrote: > On Sun, 6 Dec 2020 20:35:13 +0100 > Patrick Steinhardt wrote: >=20 > > On Fri, Dec 04, 2020 at 10:43:41AM -0600, Glenn Washburn wrote: > > > First, check to make sure that source disk has a known size. If > > > not, print debug message and return error. There are 4 cases where > > > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and > > > uboot), and in all those cases processing continues. So this is > > > probably a bit conservative. However, 3 of the cases seem > > > pathological, and the other, biosdisk, happens when booting from a > > > cd. Since I doubt booting from a LUKS2 volume on a cd is a big use > > > case, we'll error until someone complains. > > >=20 > > > Do some sanity checking on data coming from the luks header. If > > > segment.size is "dynamic", > >=20 > > Nit: there's something missing here. >=20 > Yep, thanks for catching that. I was going to complete this and forgot > in my rush to get the series out before some travel. I'll rework that. >=20 > > > Check for errors from grub_strtoull when converting segment size > > > from string. If a GRUB_ERR_BAD_NUMBER error was returned, then the > > > string was not a valid parsable number, so skip the key. If > > > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in > > > converting to a 64-bit unsigned integer. So this could be a very > > > large disk (perhaps large raid array). In this case, we want to > > > continue to try to use this key so long as the source disk's size > > > is greater than this segment size. Otherwise, this is an invalid > > > segment, and continue on to the next key. > > >=20 > > > Signed-off-by: Glenn Washburn > > > --- > > > grub-core/disk/luks2.c | 98 > > > +++++++++++++++++++++++++++++++++++++----- include/grub/disk.h | > > > 17 ++++++++ 2 files changed, 105 insertions(+), 10 deletions(-) > > >=20 > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > > > index de2e70796..1bb3a333d 100644 > > > --- a/grub-core/disk/luks2.c > > > +++ b/grub-core/disk/luks2.c > > > @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, > > > grub_luks2_digest_t *d, grub_luks2_s break; > > > } > > > if (i =3D=3D size) > > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for > > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key); > > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for > > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key);=20 > > > /* Get segment that matches the digest. */ > > > if (grub_json_getvalue (&segments, root, "segments") || > > > @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, > > > grub_luks2_digest_t *d, grub_luks2_s break; > > > } > > > if (i =3D=3D size) > > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for > > > digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key); > > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for > > > digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key);=20 > > > return GRUB_ERR_NONE; > > > } > > > @@ -600,37 +600,115 @@ luks2_recover_key (grub_disk_t source, > > > goto err; > > > } > > > =20 > > > + if (source->total_sectors =3D=3D GRUB_DISK_SIZE_UNKNOWN) > > > + { > > > + /* FIXME: Allow use of source disk, and maybe cause errors > > > in read. */ > > > + grub_dprintf ("luks2", "Source disk %s has an unknown size, " > > > + "conservatively returning error\n", > > > source->name); > > > + ret =3D grub_error (GRUB_ERR_BUG, "Unknown size of luks2 > > > source device"); > > > + goto err; > > > + } > > > + > > > /* Try all keyslot */ > > > for (i =3D 0; i < size; i++) > > > { > > > + typeof(source->total_sectors) max_crypt_sectors =3D 0; > >=20 > > Please bear with me if this has been discussed in a previous round, > > but why exactly do we cast `max_crypt_sectors` to the type of > > `source->total_sectors`? >=20 > Technically, this isn't a cast. Its a variable declaration. But I'm > using the typeof(source->total_sectors) because max_crypt_sectors can > be no more or less than the total sectors, ie its of the same type. Oh, of course. I somehow didn't realize this at all, probably because this is not something one sees that often. > > And isn't the variable always set anyway in > > case the keyslot has a non-zero priority? >=20 > Yep. Are you suggesting that it need not be initialized? This is true, > but I don't think that's a problem. I think generally initializing > things is a good practice. It might be problematic if this was in an > oft used function (or it might not, would need to see if the compiler > was smart enough to ignore the initialization). But that > initialization is going to happen very rarely in the lifetime of a grub > execution instance. I also don't really care either way. >=20 > Glenn I just didn't realize it's a declaration, so keeping the initialization is fine. Patrick --Din0jlarDx3XYfSq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl/PIBQACgkQVbJhu7ck PpRxnA/9EZ6E9lnn5fIwgO9/mpcwWYC2d4qkVGEeIqFWUKtOPtAMEY9i8Ce/hYA2 qco976XNVD5F/nB3A0S/UuxEXr4gmwla3sNJOnc3ehcVYkQfOSusirogdirKUXfo hRb+cRqTWVaU7d+Sq+6DbDLzC0PzULUMu9Cxj9NwB0l5ZcrnJUFX7xOQ4soMADPx h6Kv60J6nF8ESCeLqq2+dhueV/C+qxlI1jHMbhlSM0OgXfLqShllvGC50/M9PDX7 v/vr49gHTx4MVKY6+1mKs+cQ2AQaj5wpK/u7OVSgcD+gi2yeFwEE04ARjUEJJLxH hIQBeleOUdN9jTfcrNJMUZxHkja71LPMPiZtLESZnAConS0ET5Bs0ESzdCslvUXS tmNAR4JlGx4Nw5S/vsxqZ1qFmM9g+MHjUEKqVCsg41/e33co7YVU5RgbL9H09sOb MGb829hViKJo6Mp0R1zCztYWr7ivPOjWL+Jj5uAJv3z2H1mBMmK7OdiLQzakBEO9 0tquFux8nWpATYTRt97NwgmT8rbpYht7DiaLUxibFhMdBAmjLCixkZSOz6/XJw5v 29LBdmmnr8Zg1FSoel3zKggyZxfCianoV7701Y0WcydWXxydEDUdFzg7+R722IFI uPPeccm46eKgprD2+zA9l/WU6Ij8WimFjtxEYOex5fsx1IyPEoQ= =oYkq -----END PGP SIGNATURE----- --Din0jlarDx3XYfSq--