From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kAqWp-00068c-N9 for mharc-grub-devel@gnu.org; Wed, 26 Aug 2020 04:11:31 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60872) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kAqWn-00066V-V4 for grub-devel@gnu.org; Wed, 26 Aug 2020 04:11:30 -0400 Received: from wout1-smtp.messagingengine.com ([64.147.123.24]:50149) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kAqWl-0005Lp-78 for grub-devel@gnu.org; Wed, 26 Aug 2020 04:11:29 -0400 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id E63193DF; Wed, 26 Aug 2020 04:11:24 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Wed, 26 Aug 2020 04:11:25 -0400 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=fm1; bh=v2fUUZgc/bmIxBsaCX0lTQFM0Gd wmGq9+j+5zDU459A=; b=DCIBGXvozfODhAwJMQQLxO41NH3WI+N/uXudH8+D0+x SEu5ch8pB0gi17YHdOFshfrdoJL152ztQQyktyuZ5r/mfF1LOTkbtRlSLBAfmSsc HwGncLGvabR2O4zGsKcR4xvFsS+F+Qe/JFw1CfWI79vgJkhGSE5iDWmK+ZtxkCvg HVG2BNwii2LGCnlFJPDIw3i/waJ6EYEubXpXua1DURjeVFZkRE5Tk9kv9IT5hXha KqgpX8dHUUI30xo4NCSE36X/pK/x9uo+lhaNYnsEdQti4mYGfwpNrEcRt7CDYknF y38iKVRO9WCIIb+oXc6oHvcXForiG0BNlUDCYNjZfDQ== 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=fm3; bh=v2fUUZ gc/bmIxBsaCX0lTQFM0GdwmGq9+j+5zDU459A=; b=JLh0vcH+9H6Y0AUgjq2vH1 Rsy7oTkn37s3MuHqTm2T0qUR9TwX3q4q7vtfAXonrdm7FyftO0ZC5zgD8KxoyQY0 BO5w02NKmXzb3RDxBp0nDo7a/PebmbmdeSpLzSPnZ867kNAzhYvVtvKe1zckgfII TP8A34ZFULD3GmWX91YyLREc79Z5Emg3vOtyDaRjIYpewbIeRNtFSWUXpwgR2eyX Bte5g++Efk7i7Fz8U9YfjGmvPCOfjszKkDJzUorMrfnuDakEuv/LLyULU8rAqvq+ Hh1cXbNRJJdR8HWulZ/9JKZZ60Nhoy8sXLPuLwXSQ03PP3OFpRDJSb/ui59wgGng == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedruddvvddgtdduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucfkphepkeelrdduvddrfeekrddvfeeinecuvehluhhsthgvrhfuihiivgeptdenucfrrg hrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimh X-ME-Proxy: Received: from vm-mail.pks.im (x590c26ec.dyn.telefonica.de [89.12.38.236]) by mail.messagingengine.com (Postfix) with ESMTPA id 4D4BF328005A; Wed, 26 Aug 2020 04:11:23 -0400 (EDT) Received: from localhost (xps [10.192.0.12]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id d82c3686 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 26 Aug 2020 08:11:20 +0000 (UTC) Date: Wed, 26 Aug 2020 10:13:18 +0200 From: Patrick Steinhardt To: grub-devel@gnu.org Cc: Denis GNUtoo Carikli , Glenn Washburn , Daniel Kiper Subject: [PATCH v2 0/9] Cryptodisk fixes for v2.06 Message-ID: References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="WYTEVAkct0FjGQmd" Content-Disposition: inline In-Reply-To: Received-SPF: pass client-ip=64.147.123.24; envelope-from=ps@pks.im; helo=wout1-smtp.messagingengine.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/08/26 03:16:08 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] 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_H4=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: Wed, 26 Aug 2020 08:11:30 -0000 --WYTEVAkct0FjGQmd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, this is the second version of cryptodisk fixes which I deem to be important for the upcoming v2.06 release. Changes: - Patch 2: we're now zeroing out the UUID variable to avoid copying over uninitialized bytes. Thanks for spotting, Dennis! - Patch 3: I've replaced it with the updated version he's posted to address my feedback. - Patch 7: Updated by Glenn to now also fix writes. I didn't yet get your test series to work, Glenn. I'll try again on another day as I'm not on top of things today. Meanwhile, could you please give it a go with this updated patch series? Regards Patrick Glenn Washburn (6): luks2: Fix use of incorrect index and some error messages luks2: grub_cryptodisk_t->total_length is the max number of device native sectors cryptodisk: Unregister cryptomount command when removing module cryptodisk: Fix incorrect calculation of start sector cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt (3): json: Remove invalid typedef redefinition luks: Fix out-of-bounds copy of UUID luks2: Improve error reporting when decrypting/verifying key grub-core/disk/cryptodisk.c | 63 ++++++++++++++++++++----------------- grub-core/disk/luks.c | 8 +++-- grub-core/disk/luks2.c | 54 +++++++++++++++++-------------- grub-core/lib/json/json.h | 9 +++--- include/grub/cryptodisk.h | 2 +- include/grub/disk.h | 7 +++++ 6 files changed, 83 insertions(+), 60 deletions(-) Range-diff against v1: 1: ee402de4d =3D 1: ee402de4d json: Remove invalid typedef redefinition 2: 8668b265f ! 2: 5ecb9a4eb luks: Fix out-of-bounds copy of UUID @@ Commit message Signed-off-by: Patrick Steinhardt =20 ## grub-core/disk/luks.c ## +@@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const c= har *check_uuid, + || grub_be_to_cpu16 (header.version) !=3D 1) + return NULL; +=20 ++ grub_memset (uuid, 0, sizeof (uuid)); + optr =3D uuid; + for (iptr =3D header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.u= uid)]; + iptr++) @@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const c= har *check_uuid, newdev->source_disk =3D NULL; newdev->log_sector_size =3D 9; 3: 7cf9d9526 ! 3: f8da5b4b4 luks2: Fix use of incorrect index and some = error messages @@ Commit message Reviewed-by: Patrick Steinhardt =20 ## grub-core/disk/luks2.c ## -@@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k,= grub_luks2_digest_t *d, grub_luks2_s +@@ grub-core/disk/luks2.c: luks2_parse_digest (grub_luks2_digest_t *ou= t, const grub_json_t *digest) +=20 + static grub_err_t + luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, g= rub_luks2_segment_t *s, +- const grub_json_t *root, grub_size_t i) ++ const grub_json_t *root, grub_size_t keyslot_idx) + { + grub_json_t keyslots, keyslot, digests, digest, segments, segment; +- grub_size_t j, size; +- grub_uint64_t idx; ++ grub_size_t i, size; ++ grub_uint64_t keyslot_key, digest_key, segment_key; +=20 + /* Get nth keyslot */ + if (grub_json_getvalue (&keyslots, root, "keyslots") || +- grub_json_getchild (&keyslot, &keyslots, i) || +- grub_json_getuint64 (&idx, &keyslot, NULL) || ++ grub_json_getchild (&keyslot, &keyslots, keyslot_idx) || ++ grub_json_getuint64 (&keyslot_key, &keyslot, NULL) || + grub_json_getchild (&keyslot, &keyslot, 0) || + luks2_parse_keyslot (k, &keyslot)) +- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslo= t %"PRIuGRUB_SIZE, i); ++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslo= t index %"PRIuGRUB_SIZE, keyslot_idx); +=20 + /* Get digest that matches the keyslot. */ + if (grub_json_getvalue (&digests, root, "digests") || + grub_json_getsize (&size, &digests)) return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests"= ); - for (j =3D 0; j < size; j++) +- for (j =3D 0; j < size; j++) ++ for (i =3D 0; i < size; i++) { -- if (grub_json_getchild (&digest, &digests, i) || -+ if (grub_json_getchild (&digest, &digests, j) || + if (grub_json_getchild (&digest, &digests, i) || ++ grub_json_getuint64 (&digest_key, &digest, NULL) || grub_json_getchild (&digest, &digest, 0) || luks2_parse_digest (d, &digest)) - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"= PRIuGRUB_SIZE, i); -+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest %"= PRIuGRUB_SIZE, j); ++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest in= dex %"PRIuGRUB_SIZE, i); =20 - if ((d->keyslots & (1 << idx))) +- if ((d->keyslots & (1 << idx))) ++ if ((d->keyslots & (1 << keyslot_key))) break; } - if (j =3D=3D size) +- if (j =3D=3D size) - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keys= lot %"PRIuGRUB_SIZE); -+ return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keys= lot %"PRIuGRUB_SIZE, i); ++ if (i =3D=3D size) ++ return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keys= lot \"%"PRIuGRUB_UINT64_T"\"", keyslot_key); =20 /* Get segment that matches the digest. */ if (grub_json_getvalue (&segments, root, "segments") || grub_json_getsize (&size, &segments)) return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments= "); - for (j =3D 0; j < size; j++) -+ for (i =3D j, j =3D 0; j < size; j++) ++ for (i =3D 0; i < size; i++) { -- if (grub_json_getchild (&segment, &segments, i) || -+ if (grub_json_getchild (&segment, &segments, j) || - grub_json_getuint64 (&idx, &segment, NULL) || + if (grub_json_getchild (&segment, &segments, i) || +- grub_json_getuint64 (&idx, &segment, NULL) || ++ grub_json_getuint64 (&segment_key, &segment, NULL) || grub_json_getchild (&segment, &segment, 0) || luks2_parse_segment (s, &segment)) - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %= "PRIuGRUB_SIZE, i); -+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment %= "PRIuGRUB_SIZE, j); ++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment i= ndex %"PRIuGRUB_SIZE, i); =20 - if ((d->segments & (1 << idx))) +- if ((d->segments & (1 << idx))) ++ if ((d->segments & (1 << segment_key))) break; } - if (j =3D=3D size) +- if (j =3D=3D size) - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for diges= t %"PRIuGRUB_SIZE); -+ return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for diges= t %"PRIuGRUB_SIZE, i); ++ if (i =3D=3D size) ++ return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for diges= t \"%"PRIuGRUB_UINT64_T"\"", digest_key); =20 return GRUB_ERR_NONE; } 4: c3bf40e31 =3D 4: 150491a07 luks2: grub_cryptodisk_t->total_length is= the max number of device native sectors 5: 4b820da6c =3D 5: 7dbfad568 luks2: Improve error reporting when decry= pting/verifying key 6: f7176a87e =3D 6: dbf25a0ae cryptodisk: Unregister cryptomount comman= d when removing module 7: 13d0720d6 ! 7: 4ee7f8774 cryptodisk: Incorrect calculation of start = sector for grub_disk_read in grub_cryptodisk_read @@ Metadata Author: Glenn Washburn =20 ## Commit message ## - cryptodisk: Incorrect calculation of start sector for grub_disk_re= ad in grub_cryptodisk_read + cryptodisk: Fix incorrect calculation of start sector =20 Here dev is a grub_cryptodisk_t and dev->offset is offset in secto= rs of size native to the cryptodisk device. The sector is correctly transform= ed into @@ Commit message transformed. It would be nice if the type system would help us wit= h this. =20 Signed-off-by: Glenn Washburn + Reviewed-by: Patrick Steinhardt =20 ## grub-core/disk/cryptodisk.c ## @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk= , grub_disk_addr_t sector, @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk,= grub_disk_a err =3D grub_disk_read (dev->source_disk, - (sector << (disk->log_sector_size - - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0, -+ ((sector + dev->offset) << (disk->log_sector_size -+ - GRUB_DISK_SECTOR_BITS)), 0, - size << disk->log_sector_size, buf); +- size << disk->log_sector_size, buf); ++ grub_disk_from_native_sector (disk, sector + dev->offset), ++ 0, size << disk->log_sector_size, buf); if (err) { + grub_dprintf ("cryptodisk", "grub_disk_read failed with error %= d\n", err); +@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_write (grub_disk_t dis= k, grub_disk_addr_t sector, + } +=20 + /* Since ->write was called so disk.mod is loaded but be paranoid = */ +- =20 ++ sector =3D sector + dev->offset; + if (grub_disk_write_weak) + err =3D grub_disk_write_weak (dev->source_disk, +- (sector << (disk->log_sector_size +- - GRUB_DISK_SECTOR_BITS)) +- + dev->offset, ++ grub_disk_from_native_sector (disk, sector), + 0, size << disk->log_sector_size, tmp); + else + err =3D grub_error (GRUB_ERR_BUG, "disk.mod not loaded"); + + ## include/grub/disk.h ## +@@ include/grub/disk.h: typedef struct grub_disk_memberlist *grub_disk= _memberlist_t; + /* Return value of grub_disk_get_size() in case disk size is unknown.= */ + #define GRUB_DISK_SIZE_UNKNOWN 0xffffffffffffffffULL +=20 ++/* 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 sect= or) ++{ ++ return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS); ++} ++ + /* This is called from the memory manager. */ + void grub_disk_cache_invalidate_all (void); +=20 8: 12bc26698 =3D 8: 4aecb174b cryptodisk: Fix cipher IV mode 'plain64' = always being set as 'plain' 9: 32b463e00 ! 9: f520aecfa cryptodisk: Properly handle non-512 byte si= zed sectors @@ Commit message aestetically pleasing than desired. =20 Signed-off-by: Glenn Washburn + Reviewed-by: Patrick Steinhardt =20 ## grub-core/disk/cryptodisk.c ## @@ --=20 2.28.0 --WYTEVAkct0FjGQmd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl9GGZ0ACgkQVbJhu7ck PpQZ9xAArk28Ga7gp4fsWuFRRGAliUG14iGvzX7NxrJKXc/WKqmGq4SFLvoR4D8q pYSGod3U+2a1x4xATWz4lGheSXmuuOJQ5mA8eXk/hy8VE8CpzJCdGSj9HS0iA65m KZETu0D/uQvkvJ4Pmvt3a4WWY5c0PQXfMhQ5Do9rpE2Ij99l2AHG8hRTOUCsguld D/ZnadGbBCu8Kjjr3k5zrnjjuobBsC+coo20taQSqyWLCBbRKV0QpEGl7O9NbwWa LzUB4HEhRnurkRdYykZ+kTzC4E8pPAuEW6sjMgS1FNr3P1WcJBxe4x52l3iWPgIs PqvB3bqs2pU9YhUYVcbvknAnPZJlKYQdo39bxaVEk+EYWmfCTEv9ThYGHQgbY6UA StQF+MzTSWc9b52cNCoj//gIOvmellDUPPinG1Fc8Jti19Rw7UWtCgkKdPToz+N+ dx9ic6lq0ofvhShzMpfVQnjp8EAAKd7b8LI0GuRqna/5JehzwDN1OHuJWmIyI6Fk ILARrPks6ZimEr90JTflzWxWn89w3xWVenRzWWYW2SHDJOJP85PWGdIiAwAsSNyz 4jo8DvWHWESbTlMSiFHzam0NCn2z1J2dMm+sAsF8kual8bLA6x/LVRvuzZroLUui 1WUBpgq2xNYwFt3vYiOxUVy1o2wKBv+QVXFUewcK7ntPZcVLGQU= =yzG1 -----END PGP SIGNATURE----- --WYTEVAkct0FjGQmd--