From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1oSXU5-0007FK-3D for mharc-grub-devel@gnu.org; Mon, 29 Aug 2022 01:38:53 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:57374) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oSXU0-0007Es-Un for grub-devel@gnu.org; Mon, 29 Aug 2022 01:38:49 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:45057) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oSXTy-0005LM-I2 for grub-devel@gnu.org; Mon, 29 Aug 2022 01:38:48 -0400 Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 3E4D25C00BD; Mon, 29 Aug 2022 01:38:42 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute4.internal (MEProxy); Mon, 29 Aug 2022 01:38:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=cc:cc :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm1; t=1661751522; x=1661837922; bh=E3wTBdA1IH wxm2sql/9vK2BdDgpCPwD93FzJPXCzwVw=; b=nOTkEftcbB1+TNIK/LgEqSTLYj Zy277MU+yVSJlehx6iuHbQ9BUx11QJOgPc/ab2riCetCNMa5ItC4zlB8IW0Fm6sT C3VnSkRPX6rNVfKuJo7vHGJzXlCv7t+lzQjk8cXt5rW5ITpMHMRi4jfVGHa2fTC3 Dllj3nfRQsSF7patsKAwiDLmOfWjnwOHnmAhtN2vKKvff3NX65wH/ngBHxbIi02p QZkXVTS+D585OGEJeN/96vxn/tMksDhTFvbDqkUF0sub36KIRDJPM5Qltwq7XJp9 QsWTG/Z1GLb04tttyi96R/2Ku14qpAs/5PdeNedOHLI6cHiU0fWGLYcbw3EA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm1; t=1661751522; x=1661837922; bh=E3wTBdA1IHwxm2sql/9vK2BdDgpC PwD93FzJPXCzwVw=; b=LeFZWqWuw5zR7bjkNFPJGml3rd6suwuVa8jywyl8LIWX /UC1QOjrpufWu+Y+8Vk4mX2MevVPtlOfwqkPHdNdT2GMtrJFCcVuhiHecnwsEbhQ 5v4zDaPllLEoufyavZS+pO3YgI/fC09wdCLR7gslip3VuPBESHYiKnWVnOcvFbVV X8wFKvWxtyvVlHkpH009HnUkSfs7d4ndXM9L7zF6eOkRZSUvIaA2NhLcNXIHmGtX GIyUdN47M5OTVNDxrBTand4LLPJaBu15zE4bJXCNQ2L7bGOWCQ7T1Q6+Y1RQGkTv Fl7+3NN3zbg1YpzU5Lx8Gvaf91zUtjq2a8D4G1hHCg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrvdektddguddttdcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrght rhhitghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtth gvrhhnpeeukedtvedtffevleejtefgheehieegkeeluddvfeefgeehgfeltddtheejleff teenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpsh esphhkshdrihhm X-ME-Proxy: Feedback-ID: i197146af:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 29 Aug 2022 01:38:41 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 80ebe661 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Mon, 29 Aug 2022 05:38:34 +0000 (UTC) Date: Mon, 29 Aug 2022 07:38:24 +0200 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper Subject: Re: [PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner Message-ID: References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="0PHAhSYTrldPiYyk" Content-Disposition: inline In-Reply-To: Received-SPF: pass client-ip=66.111.4.29; envelope-from=ps@pks.im; helo=out5-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, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 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: Mon, 29 Aug 2022 05:38:49 -0000 --0PHAhSYTrldPiYyk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote: > A user can now specify UUID strings with dashes, instead of having to rem= ove > dashes. This is backwards-compatability preserving and also fixes a source > of user confusion over the inconsistency with how UUIDs are specified > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the > reference implementation for LUKS, displays and generates UUIDs with dash= es > there has been additional confusion when using the UUID strings from > cryptsetup as exact input into GRUB does not find the expected cryptodisk. >=20 > A new function grub_uuidcasecmp is added that is general enough to be used > other places where UUIDs are being compared. >=20 > Signed-off-by: Glenn Washburn > --- > grub-core/disk/cryptodisk.c | 4 ++-- > grub-core/disk/geli.c | 2 +- > grub-core/disk/luks.c | 21 ++++----------------- > grub-core/disk/luks2.c | 15 ++++----------- > include/grub/misc.h | 32 ++++++++++++++++++++++++++++++++ > 5 files changed, 43 insertions(+), 31 deletions(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index e89430812..a4cd8445f 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t d= isk) > if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) =3D= =3D 0) > { > for (dev =3D cryptodisk_list; dev !=3D NULL; dev =3D dev->next) > - if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) =3D= =3D 0) > + if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, siz= eof (dev->uuid)) =3D=3D 0) > break; > } > else > @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid) > { > grub_cryptodisk_t dev; > for (dev =3D cryptodisk_list; dev !=3D NULL; dev =3D dev->next) > - if (grub_strcasecmp (dev->uuid, uuid) =3D=3D 0) > + if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) =3D=3D 0) > return dev; > return NULL; > } > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index e190066f9..722910d64 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t = cargs) > return NULL; > } > =20 > - if (cargs->search_uuid !=3D NULL && grub_strcasecmp (cargs->search_uui= d, uuid) !=3D 0) > + if (cargs->search_uuid !=3D NULL && grub_uuidcasecmp (cargs->search_uu= id, uuid, sizeof (uuid)) !=3D 0) > { > grub_dprintf ("geli", "%s !=3D %s\n", uuid, cargs->search_uuid); > return NULL; > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 7f837d52c..9e1e6a5d9 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -66,10 +66,7 @@ static grub_cryptodisk_t > luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs) > { > grub_cryptodisk_t newdev; > - const char *iptr; > struct grub_luks_phdr header; > - char *optr; > - char uuid[sizeof (header.uuid) + 1]; > char ciphername[sizeof (header.cipherName) + 1]; > char ciphermode[sizeof (header.cipherMode) + 1]; > char hashspec[sizeof (header.hashSpec) + 1]; > @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t c= args) > || 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.uuid= )]; > - iptr++) > + if (cargs->search_uuid !=3D NULL && grub_uuidcasecmp (cargs->search_uu= id, header.uuid, sizeof (header.uuid)) !=3D 0) > { > - if (*iptr !=3D '-') > - *optr++ =3D *iptr; > - } > - *optr =3D 0; > - > - if (cargs->search_uuid !=3D NULL && grub_strcasecmp (cargs->search_uui= d, uuid) !=3D 0) > - { > - grub_dprintf ("luks", "%s !=3D %s\n", uuid, cargs->search_uuid); > + grub_dprintf ("luks", "%s !=3D %s\n", header.uuid, cargs->search_u= uid); > return NULL; > } I haven't noticed this in my previous review round, but I think this is wrong because `header.uuid` is never NUL-terminated. It is explicitly 40 characters long and read directly from disk, so there wouldn't be any room for it to be NUL-terminated. So we'd rather have to: grub_dprintf ("luks2", "%.*s !=3D %s\n", header.uuid, sizeof (header.uu= id), cargs->search_uuid); Sorry for not catching this in my first round. > =20 > @@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t = cargs) > newdev->source_disk =3D NULL; > newdev->log_sector_size =3D GRUB_LUKS1_LOG_SECTOR_SIZE; > newdev->total_sectors =3D grub_disk_native_sectors (disk) - newdev->of= fset_sectors; > - grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); > + grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid)); > newdev->modname =3D "luks"; This should be fine though because `newdev->uuid` is initialized to all-zeroes. > /* Configure the hash used for the AF splitter and HMAC. */ > @@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t = cargs) > return NULL; > } > =20 > - COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >=3D sizeof (uuid)); > + COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >=3D sizeof (header.uuid)); > return newdev; > } This has an off-by-one bug now though: `sizeof(uuid)` is not the same as `sizeof(header.uuid)`, but instead it was `sizeof(header.uuid)+1` to account for the trailing NUL-byte. So we have to make this: COMPILE_TIME_ASSERT (sizeof (newdev->uuid) > sizeof (header.uuid)); > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index 5b3b36c8a..1174c4c2b 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t= cargs) > { > grub_cryptodisk_t cryptodisk; > grub_luks2_header_t header; > - char uuid[sizeof (header.uuid) + 1]; > - grub_size_t i, j; > =20 > if (cargs->check_boot) > return NULL; > @@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_= t cargs) > return NULL; > } > =20 > - for (i =3D 0, j =3D 0; i < sizeof (header.uuid); i++) > - if (header.uuid[i] !=3D '-') > - uuid[j++] =3D header.uuid[i]; > - uuid[j] =3D '\0'; > - > - if (cargs->search_uuid !=3D NULL && grub_strcasecmp (cargs->search_uui= d, uuid) !=3D 0) > + if (cargs->search_uuid !=3D NULL && grub_uuidcasecmp (cargs->search_uu= id, header.uuid, sizeof (header.uuid)) !=3D 0) > { > - grub_dprintf ("luks2", "%s !=3D %s\n", uuid, cargs->search_uuid); > + grub_dprintf ("luks2", "%s !=3D %s\n", header.uuid, cargs->search_= uuid); > return NULL; > } Same here. > @@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t= cargs) > if (!cryptodisk) > return NULL; > =20 > - COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >=3D sizeof (uuid)); > - grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid)); > + COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >=3D sizeof (header.uui= d)); > + grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid)); And here. Patrick > cryptodisk->modname =3D "luks2"; > return cryptodisk; > diff --git a/include/grub/misc.h b/include/grub/misc.h > index 1c25c1767..76c5c7992 100644 > --- a/include/grub/misc.h > +++ b/include/grub/misc.h > @@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, gr= ub_size_t n) > - (int) grub_tolower ((grub_uint8_t) *s2); > } > =20 > +/* > + * Do a case insensitive compare of two UUID strings by ignoring all das= hes. > + * Note that the parameter n, is the number of significant characters to > + * compare, where significant characters are any except the dash. > + */ > +static inline int > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n) > +{ > + if (n =3D=3D 0) > + return 0; > + > + while (*uuid1 && *uuid2 && --n) > + { > + /* Skip forward to non-dash on both UUIDs. */ > + while ('-' =3D=3D *uuid1) > + ++uuid1; > + > + while ('-' =3D=3D *uuid2) > + ++uuid2; > + > + if (grub_tolower ((grub_uint8_t) *uuid1) > + !=3D grub_tolower ((grub_uint8_t) *uuid2)) > + break; > + > + uuid1++; > + uuid2++; > + } > + > + return (int) grub_tolower ((grub_uint8_t) *uuid1) > + - (int) grub_tolower ((grub_uint8_t) *uuid2); > +} > + > /* > * Note that these differ from the C standard's definitions of strtol, > * strtoul(), and strtoull() by the addition of two const qualifiers on = the end > --=20 > 2.34.1 >=20 --0PHAhSYTrldPiYyk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmMMUM8ACgkQVbJhu7ck PpQ0oBAAq0EV83CLCck34wbilnEuOrHGuE7apxkLBnuFTD7d1wMtXcbFJadX8IY/ Dr07ZCxYtxBkAUY91RgFV934Shb1kU2X+QbYHpTf6e2E9lKpFNHUZxevqtRUI2+5 2jV3x5/qNwJ6dkzBZTd14p7qqpkqAz1O/Wup+hGxRbpd28NaPTvrnUai7OHO4BM5 62IZueGgXj7xfuBKopMcu3YsJeQ2QGj+JDbDxqHHxuUbyHHc6R1pdxENb0Ll7Ecr qD0tMwh1728ozyBfJR0I4MgPL9Ii09Th2pN7Jx5BI3gKAkAIh1HOJhYH+NIEedqP priPlbZ8+9i1M6Q5X5xVqfMH7FVr9RQJSX0MYQr91rjUqFxGACKSUXF82If6DXtq NwaxIVqLbnA+CIvg6x8+aw3CmhKxvUaPrTfkBAGJuDnoSOMjLrK4EbH93CH64EZw oeG8gXKzfDWyG8m1WhuqC3tvcYSm0PUVZCpM+1DH72NRE4lmhyp+D13SeSJmydbP fetn7mm+Mf4l57Q/GyWtpiXYH2e14tvN4IIdshPwrPYdZ7+b7SyLwi700xTl0AWm tIuaNLItEsDIXZRAdY++zPzmyJXz7W9rfkrAD34SOujOmQNIjUbiB8gASiprd5Bx FlWkhWGgcQ3PPaQtvaTxikW6S2QT/lddZ69A28CcLkqrNq1ROx4= =iU/I -----END PGP SIGNATURE----- --0PHAhSYTrldPiYyk--