From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mX7xE-0000Ri-47 for mharc-grub-devel@gnu.org; Sun, 03 Oct 2021 16:19:25 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46704) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mX7xC-0000RX-VE for grub-devel@gnu.org; Sun, 03 Oct 2021 16:19:22 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:37713) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mX7x8-0000Z3-N9 for grub-devel@gnu.org; Sun, 03 Oct 2021 16:19:22 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 5BD325C0121; Sun, 3 Oct 2021 16:19:17 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Sun, 03 Oct 2021 16:19:17 -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=fm2; bh=eIwg7UsaNLVMV4pbF0JVD+OAewE I5kr31EDN2oEgCls=; b=fkc7pIWgMCtGO65daTo2lRen63YMNIvXAUVerpEF9W4 hl2KAr50YevNdEcMXOqpRQJ8Qy4MniQSxukWbLZv9eP3JunFEZo7uUDdk6lzdXsJ kKZNHz42AyOryQ3OXV34bMqDgFAzGcxKG9utm9Eldwu/ygpZjz4KnmR4Y7IKYa7m 5MqNwBiQDqRMAS7BUVRpj6GYRxDEptGT3oadyOrkZrqFcuU5AEUIJWd90Q0jMQXh +2D2Fy0na+gnv3ETGm7tsqiQ+9pt/ZhSIrl46ZLowNtvhI1M6TLXnHRNa5PjEBkI UgterknGnCidLtuWsMz9s8b3tryq86W1nGuWy+dbGAQ== 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=eIwg7U saNLVMV4pbF0JVD+OAewEI5kr31EDN2oEgCls=; b=BgtBdDgl9odAcmI4eNfN/t G/Mn6grwQArsoJjSGdFtkNkHLYRKLiwMUmWN1qjfU7LHW3oq1y0hMbQ0NSSqx+Jt FJPjN7X/i3idFvs2ePWAVpM/czNZXt1wEPANz28t+lkX99HCKI2UHtrQRFe/IQNE SUPaoZD/fFsDOoAdQDHFhv0mLPEug6WRTJG3HXeczvZN0OUb9de8/YaDk8jKcZri 4+WAcu7NsFPqVMI5Y2/s1WKk5ufDu3D5sO/XJ5Ip6zbuT4i3Xs9N9D0RNtqNLdmG MzHFBteFCyx+ZfMdmpYo8SBsUFrpVzxuPYt0fbSG0jtCtAHvP1/CU6187+TN8QLw == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudeltddgudeghecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecuggftrfgrthhtvg hrnhepheeghfdtfeeuffehkefgffduleffjedthfdvjeektdfhhedvlefgtefgvdettdfh necuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepphhsse hpkhhsrdhimh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 3 Oct 2021 16:19:16 -0400 (EDT) Received: from localhost (tanuki [10.192.0.23]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id fbd5496f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 3 Oct 2021 20:19:14 +0000 (UTC) Date: Sun, 3 Oct 2021 22:22:40 +0200 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper , Denis 'GNUtoo' Carikli , James Bottomley Subject: Re: [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args Message-ID: References: <20210927231403.642857-1-development@efficientek.com> <20210927231403.642857-5-development@efficientek.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="JauTQ3fzOQPfFcFm" Content-Disposition: inline In-Reply-To: <20210927231403.642857-5-development@efficientek.com> 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, RCVD_IN_MSPIKE_H2=-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: Sun, 03 Oct 2021 20:19:23 -0000 --JauTQ3fzOQPfFcFm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote: > The member found_uuid was never used by the crypto-backends, but was used= to > determine if a crypto-backend successfully mounted a cryptodisk with a gi= ven > uuid. This is not needed however, because grub_device_iterate will return= 1 > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device > will only return 1 if a search_uuid has been specified and a cryptodisk w= as > successfully setup by a crypto-backend. So the return value of > grub_cryptodisk_scan_device is equivalent to found_uuid. Is that always the case though? Most notably, `scan_device_real` will only set `have_it =3D 1` in case we have recovered the key. If the cryptodisk existed before and was found via `get_by_source_disk`, then we wouldn't set `have_it` at all. This essentially means that we'd only set `have_it` in case we found a new cryptodisk, but not if we return a preexisting one. I don't know whether this behaviour is something we rely on. My gut feeling says it's rather a bug in the current code, which seems to be confirmed by the error message in the `if (!have_it)` case, which says "no such cryptodisk found". We did find one, but it's not a new one. So in total I think your patch makes sense and fixes a bug, but the description doesn't quite match reality. Patrick > Signed-off-by: Glenn Washburn > --- > grub-core/disk/cryptodisk.c | 9 ++++----- > include/grub/cryptodisk.h | 1 - > 2 files changed, 4 insertions(+), 6 deletions(-) >=20 > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 5e153ee0a..033894257 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name, > =20 > grub_cryptodisk_insert (dev, name, source); > =20 > - cargs->found_uuid =3D 1; > - > goto cleanup; > } > goto cleanup; > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name, > =20 > if (err) > grub_print_error (); > - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0; > + return (!err && cargs->search_uuid) ? 1 : 0; > } > =20 > static grub_err_t > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i= nt argc, char **args) > =20 > if (state[0].set) /* uuid */ > { > + int found_uuid =3D 0; > grub_cryptodisk_t dev; > =20 > dev =3D grub_cryptodisk_get_by_uuid (args[0]); > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i= nt argc, char **args) > =20 > cargs.check_boot =3D state[2].set; > cargs.search_uuid =3D args[0]; > - grub_device_iterate (&grub_cryptodisk_scan_device, &cargs); > + found_uuid =3D grub_device_iterate (&grub_cryptodisk_scan_device, = &cargs); > =20 > - if (!cargs.found_uuid) > + if (!found_uuid) > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found"); > return GRUB_ERR_NONE; > } > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > index 230167ab0..f4afb9cbd 100644 > --- a/include/grub/cryptodisk.h > +++ b/include/grub/cryptodisk.h > @@ -70,7 +70,6 @@ typedef gcry_err_code_t > struct grub_cryptomount_args > { > grub_uint32_t check_boot : 1; > - grub_uint32_t found_uuid : 1; > char *search_uuid; > grub_uint8_t *key_data; > grub_size_t key_len; > --=20 > 2.32.0 >=20 --JauTQ3fzOQPfFcFm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIyBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmFaEQ8ACgkQVbJhu7ck PpRsRw/1EaicnW3TsrhZ6KUbIsTwreuTHcu52dy0e25NnWoCQPqbwv8LWYnLViwj 4afrLom3719AVYsbXM6jXkDTPgWrGG618heMWc1BcJcGeD/kSuh3S5246urVKoKm HIWQtgTeekxaJ6O5enOb1ySUVvHRq3F7hbFvBSmxNNSVfPIyFNm25V21Hc4y14HV V3o8g6JfJEeyX/Ktk9JnK8x/eGAle+B1dBvaxU7dBht20rRE9QbtTVqdm9u5Idwr qWQMIwLe0jYzMgdkaQZ5Z5eKKePahukb94hOWOGkVYKJSBsxGNpkeECdqApQltQn 9GGzUfNH/jabICZmpfrJu0Ub7dF+u7iqEEY33RfyQMIrng3Kbvd5DRnjKC9PCBKo pXd903eRFf0MFIkU8UisKem37uln/7pBoc8cVUVXyCyaJ2iEk19RmGMfOaw6J6zT rb+oE5h5VnXLWhZbD/AKWNoQvJkptVqksHCMGTIqcr2/rHGMp5BPD7Fn0XzenBp7 RIAOF8CJ0V5DcLwLVmg/biEODf0g/cTVdvw0rXn8p+OiLZ4/NYHH8eY3KW63gs7O 0R/FCO+H27wCD8jDqpb0SWPD48qgn3kG5+i6KZ1wuTIm+ZZgUAXIxrpNGjmngUK8 aWvXupS2JIuiIvHRfpVNg9jGb5iqowKJZrUYnGqMxDNRGaiU0w== =XPcy -----END PGP SIGNATURE----- --JauTQ3fzOQPfFcFm--