From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1klzon-00078d-Ew for mharc-grub-devel@gnu.org; Sun, 06 Dec 2020 14:35:37 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:50926) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1klzoj-00073i-Ma for grub-devel@gnu.org; Sun, 06 Dec 2020 14:35:33 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:60109) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1klzoX-0006Fq-Qv for grub-devel@gnu.org; Sun, 06 Dec 2020 14:35:33 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 3D7555C00B9; Sun, 6 Dec 2020 14:35:19 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Sun, 06 Dec 2020 14:35:19 -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=iNXTyK6KOSoMwni2+Yeb8qGa3cM P5dv6OLV1J/Z/DzQ=; b=gHcwBixl5HVEZm2RxZkqOnFBmdmfMxFRGb4u7aPJO6b HVmhOY/AixTEakCOuaIHblqzQVPyYsG7sfsuWGfshJmdwwcVvDnEWmo/qeKNkm4N G/yuVf6Owxp9KA6g5xdbMulu345ThYGfmPXZGtFS42hrlAe8+02GrkjXd7Y78yEs xbxym/nwCe10G4QVrHndyzwVoELFixUCzAoIUytsmErzN7kdM8dyVjhhYbDOlf2y 6oiTpgMsWtkp7cVgYv18HYmR/arceTVSxZsW1ICzcc9DXJY9Ids6s3OOFYQaK/PZ wUodKGLj5QKYYORw7DZ/PQsK0hdtivWd4QLlQt76sXQ== 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=iNXTyK 6KOSoMwni2+Yeb8qGa3cMP5dv6OLV1J/Z/DzQ=; b=Yk/7nByMQuVusZAVTZHyNY QB5JhK32WDmz/LlunpKQly/XMfBghQtJ9toT5tU0mEUzUyiFy04+XFAzfgKCCeUS 281Xt0t+o8tiawv3J/k7L2VN28LMQfR3dQ3X0TBl5pc2bUntxh3yWTDNI0yB87Db d9BjN8jPiFkzbD+FgRax+pA5lZT6PYAD7ugaz/Ly+jo99c+V1oSyeq0Z2pl6640J VrPSqvm8uJC0T6af7plCWX951mJPpCMT9DkyKerwJhVwm/XeLi/ipUy2LTwFpWkE Xx0ucheMsUwNGwvtk8uptGoU/U8UwGFzfAUcdJ+YTfi0LUEDK0xYzqJVrvL7zNzA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrudejvddguddvkecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvg hrnhepheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfh necukfhppeejjedrudekfedrudejrdduuddtnecuvehluhhsthgvrhfuihiivgeptdenuc frrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Received: from vm-mail (x4db7116e.dyn.telefonica.de [77.183.17.110]) by mail.messagingengine.com (Postfix) with ESMTPA id 34F78108005B; Sun, 6 Dec 2020 14:35:18 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail (OpenSMTPD) with ESMTPSA id 690a29a1 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 6 Dec 2020 19:35:15 +0000 (UTC) Date: Sun, 6 Dec 2020 20:35:13 +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: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="E86g0IpPpJYKhVeZ" Content-Disposition: inline In-Reply-To: Received-SPF: pass client-ip=66.111.4.28; envelope-from=ps@pks.im; helo=out4-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.01, RCVD_IN_MSPIKE_WL=-0.01, 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: Sun, 06 Dec 2020 19:35:34 -0000 --E86g0IpPpJYKhVeZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 LU= KS2 > 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.s= ize > is "dynamic", Nit: there's something missing here. > 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_luks= 2_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_luks= 2_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= =2E */ > + 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 de= vice"); > + goto err; > + } > + > /* Try all keyslot */ > for (i =3D 0; i < size; i++) > { > + typeof(source->total_sectors) max_crypt_sectors =3D 0; 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`? And isn't the variable always set anyway in case the keyslot has a non-zero priority? Patrick > + > + grub_errno =3D GRUB_ERR_NONE; > ret =3D luks2_get_keyslot (&keyslot, &digest, &segment, json, i); > if (ret) > goto err; > + if (grub_errno !=3D GRUB_ERR_NONE) > + grub_dprintf ("luks2", "Ignoring unhandled error %d from luks2_get_ke= yslot\n", grub_errno); > =20 > if (keyslot.priority =3D=3D 0) > { > - grub_dprintf ("luks2", "Ignoring keyslot \"%"PRIuGRUB_UINT64_T"\" due= to priority\n", keyslot.slot_key); > + grub_dprintf ("luks2", "Ignoring keyslot \"%"PRIuGRUB_UINT64_T"\" due= to priority\n", keyslot.json_slot_key); > continue; > } > =20 > - grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n"= , keyslot.slot_key); > + grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n"= , keyslot.json_slot_key); > =20 > /* Set up disk according to keyslot's segment. */ > crypt->offset_sectors =3D grub_divmod64 (segment.offset, segment.s= ector_size, NULL); > crypt->log_sector_size =3D sizeof (unsigned int) * 8 > - __builtin_clz ((unsigned int) segment.sector_size) - 1; > + /* Set to the source disk size, which is the maximum we allow. */ > + max_crypt_sectors =3D grub_disk_convert_sector(source, > + source->total_sectors, > + crypt->log_sector_size); > + > + if (max_crypt_sectors < crypt->offset_sectors) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has offset" > + " %"PRIuGRUB_UINT64_T" which is greater than" > + " source disk size %"PRIuGRUB_UINT64_T"," > + " skipping\n", > + segment.json_slot_key, crypt->offset_sectors, > + max_crypt_sectors); > + continue; > + } > + > if (grub_strcmp (segment.size, "dynamic") =3D=3D 0) > - crypt->total_sectors =3D (grub_disk_get_size (source) >> (crypt->log_se= ctor_size - source->log_sector_size)) > - - crypt->offset_sectors; > + crypt->total_sectors =3D max_crypt_sectors - crypt->offset_sectors; > else > - crypt->total_sectors =3D grub_strtoull (segment.size, NULL, 10) >> cryp= t->log_sector_size; > + { > + grub_errno =3D GRUB_ERR_NONE; > + /* Convert segment.size to sectors, rounding up to nearest sector */ > + crypt->total_sectors =3D grub_strtoull (segment.size, NULL, 10); > + crypt->total_sectors =3D ALIGN_UP (crypt->total_sectors, > + 1 << crypt->log_sector_size); > + crypt->total_sectors >>=3D crypt->log_sector_size; > + > + if (grub_errno =3D=3D GRUB_ERR_NONE) > + ; > + else if (grub_errno =3D=3D GRUB_ERR_BAD_NUMBER) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size" > + " \"%s\" is not a parsable number\n", > + segment.json_slot_key, segment.size); > + continue; > + } > + else if (grub_errno =3D=3D GRUB_ERR_OUT_OF_RANGE) > + { > + /* > + * There was an overflow in parsing segment.size, so disk must > + * be very large or the string is incorrect. > + */ > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size" > + " %s overflowed 64-bit unsigned integer," > + " the end of the crypto device will be" > + " inaccessible\n", > + segment.json_slot_key, segment.size); > + if (crypt->total_sectors > max_crypt_sectors) > + crypt->total_sectors =3D max_crypt_sectors; > + } > + } > + > + if (crypt->total_sectors =3D=3D 0) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has zero" > + " sectors, skipping\n", > + segment.json_slot_key); > + continue; > + } > + else if (max_crypt_sectors < (crypt->offset_sectors + crypt->total= _sectors)) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has last" > + " data position greater than source disk size," > + " the end of the crypto device will be" > + " inaccessible\n", > + segment.json_slot_key); > + /* Allow decryption up to the end of the source disk. */ > + crypt->total_sectors =3D max_crypt_sectors - crypt->offset_sectors; > + } > =20 > ret =3D luks2_decrypt_key (candidate_key, source, crypt, &keyslot, > (const grub_uint8_t *) passphrase, grub_strlen (passphrase)); > if (ret) > { > grub_dprintf ("luks2", "Decryption with keyslot \"%"PRIuGRUB_UINT64_T= "\" failed: %s\n", > - keyslot.slot_key, grub_errmsg); > + keyslot.json_slot_key, grub_errmsg); > continue; > } > =20 > @@ -638,7 +716,7 @@ luks2_recover_key (grub_disk_t source, > if (ret) > { > grub_dprintf ("luks2", "Could not open keyslot \"%"PRIuGRUB_UINT64_T"= \": %s\n", > - keyslot.slot_key, grub_errmsg); > + keyslot.json_slot_key, grub_errmsg); > continue; > } > =20 > @@ -646,7 +724,7 @@ luks2_recover_key (grub_disk_t source, > * TRANSLATORS: It's a cryptographic key slot: one element of an a= rray > * where each element is either empty or holds a key. > */ > - grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"), keyslo= t.slot_key); > + grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"), keyslo= t.json_slot_key); > =20 > candidate_key_len =3D keyslot.key_size; > break; > diff --git a/include/grub/disk.h b/include/grub/disk.h > index 0fb727d3d..f9227f285 100644 > --- a/include/grub/disk.h > +++ b/include/grub/disk.h > @@ -27,6 +27,8 @@ > #include > /* For NULL. */ > #include > +/* For ALIGN_UP. */ > +#include > =20 > /* These are used to set a device id. When you add a new disk device, > you must define a new id for it here. */ > @@ -174,6 +176,21 @@ typedef struct grub_disk_memberlist *grub_disk_membe= rlist_t; > /* Return value of grub_disk_native_sectors() in case disk size is unkno= wn. */ > #define GRUB_DISK_SIZE_UNKNOWN 0xffffffffffffffffULL > =20 > +/* Convert sector number from disk sized sectors to a log-size sized sec= tor. */ > +static inline grub_disk_addr_t > +grub_disk_convert_sector (grub_disk_t disk, > + grub_disk_addr_t sector, > + grub_size_t log_sector_size) > +{ > + if (disk->log_sector_size < log_sector_size) > + { > + sector =3D ALIGN_UP (sector, 1 << (log_sector_size / disk->log_sec= tor_size)); > + return sector >> (log_sector_size - disk->log_sector_size); > + } > + else > + return sector << (disk->log_sector_size - log_sector_size); > +} > + > /* Convert to GRUB native disk sized sector from disk sized sector. */ > static inline grub_disk_addr_t > grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t sector) > --=20 > 2.27.0 >=20 --E86g0IpPpJYKhVeZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl/NMnAACgkQVbJhu7ck PpR6XhAAnsdOfABHo36gfJSau89DwjWiGuI9DRlfPceo1/Owx920I2lyQy08ZusS OTwmIh339W9Cn77IoGOft2pZveerK8RqQhxTziT1kp4vt4gvEMeLJ5dHv1zTGZw6 38Dzbd/rnT3qkf0jGnYwcY4k7Ik9b2OEkCc+2FJ/17rC7KO9q2PrixWb1CqRnGsI zsEpUzlb1FQfmSnZSChrAAAuoApxSxCFXjYIPnTWIB5MnllB5Ioo/WqAnLKDPPWG TGVOhc1zARTyHst7rqQ3j9IdcbqcV3ESWR3IuK1V+XPbMWS8skxKgIguGmOmgIzz p0I/ce81ZqPF3sR44BGXculsUSlud+52F3g6XQCa8wERuSQHdLftv1pbTQ60UTY4 rhRsMRivqeCUUI5xwxuRzsEQGD1ntTLb0yEgDI3EFTr8mqYeUBya8UQhPAgdQo9U 2CLG3WVk93mukVcVHfqr1F9+kMkHGopkUYjeBC9NSgrtoHl2v+Jkn6W07SXGiwTd T63necXXnrWJR+OkVYQwX7K1TARaPuiVy+NHcYH+fOGelWXL/tMJ3IUdOc9di3/+ H6SAXS5b0CgYsUZI7s70ZfRBmM9GoZt7Xedv//0MM7HXV3AAYW6LMViZIljzBke+ 6q7PSKvezUkXvtTRnW9mcVkbqbXv/eFrEeWjSG50PtcNRLjcJzs= =lg4y -----END PGP SIGNATURE----- --E86g0IpPpJYKhVeZ--