From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mZTta-0002w6-4u for mharc-grub-devel@gnu.org; Sun, 10 Oct 2021 04:09:22 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41804) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mZTtY-0002vv-Oy for grub-devel@gnu.org; Sun, 10 Oct 2021 04:09:21 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:56153) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mZTtS-0001JD-BA for grub-devel@gnu.org; Sun, 10 Oct 2021 04:09:20 -0400 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 905CF5C0105; Sun, 10 Oct 2021 04:09:11 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Sun, 10 Oct 2021 04:09:11 -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=uSbcdQVg7xGANJApGfl/zt13eTM /MUHK+GMBZCvhSmc=; b=Q5NDs5FGWqJ/82UBh1fW5jywwYdnpnjekhI5UZ5YUfN INNa8QKfRYusEmYzU15tDuAogHDKIlX6z414RLzFzmm4KOQMVf+Se30cO/mLGh1P yCdMxJz1fxNz3bqeq5C6HbqY6Cgni3JvxpjRDm6x8p2uimAfoBKH/gnwScBP3GWM 1AGzLHCTqerevZMM4PQUXgtZZ2fZu45KLmXThbYhsi3qEs8pXLIiGqKQDLnZ4DKG DPdcyNR2oD3bx6FvfPYLyNJx5Jmvnuidup5/nk9mmSG7fZ2lA7476g4wTRlBdocx cF2CGNiF244wOSI8BMmJEHY+p/rQlHuspk5SzO2bN2w== 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=uSbcdQ Vg7xGANJApGfl/zt13eTM/MUHK+GMBZCvhSmc=; b=S9uDrmSAqBcWCBX/3uQ4Op 8BdI7elO1DI+ynbo1jm2lBvOvsFjDNxvl84TRpRJvkCjQ3JXGl4zFgf+zp7zq9aL Lruj9y2cK6ACLNnmFbg1GipsgzJV0bDup85B08+CvtwOQVUdPJgC3B1s3MNDU996 XAPIZujyUdUS19AsKeH7vYW/QfFDqGmQiq6VGvT3s07SAFmGQDtjVjc3HLNoYP75 BWXCuJT1mDG3GUNKqUJR61XthhFkUer6MbFp7jodvIR0R4bOcnYyD77JilzfRV23 F5qMTrRyhj8kJKnMLO0D5a9yJJ9GJUts/fboldhTEdG6yxtF2WmuljyorOBxfXPg == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvddtgedgtdduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sun, 10 Oct 2021 04:09:10 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 0d829aeb (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Sun, 10 Oct 2021 08:09:06 +0000 (UTC) Date: Sun, 10 Oct 2021 10:09:05 +0200 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper Subject: Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct Message-ID: References: <20210907023430.2eaff8a1@ubuntu> <20210913210515.5753cf48@ubuntu> <20211004133218.685b2929@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="PDwWNm7NLfMtO6Hw" Content-Disposition: inline In-Reply-To: <20211004133218.685b2929@crass-HP-ZBook-15-G2> 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, 10 Oct 2021 08:09:21 -0000 --PDwWNm7NLfMtO6Hw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Oct 04, 2021 at 11:51:33PM -0500, Glenn Washburn wrote: > On Mon, 4 Oct 2021 10:55:21 +0200 > Patrick Steinhardt wrote: >=20 > > On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote: > > > On Sun, 12 Sep 2021 13:17:29 +0200 > > > Patrick Steinhardt wrote: > > >=20 > > > > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote: > > > > > On Mon, 30 Aug 2021 20:02:26 +0200 > > > > > Patrick Steinhardt wrote: > > > > >=20 > > > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote: > > > > > > > Signed-off-by: Glenn Washburn > > > > > > > --- > > > > > > > grub-core/disk/cryptodisk.c | 26 +++++++++----------------- > > > > > > > include/grub/cryptodisk.h | 3 +++ > > > > > > > 2 files changed, 12 insertions(+), 17 deletions(-) > > > > > > >=20 > > > > > > > diff --git a/grub-core/disk/cryptodisk.c > > > > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 1006= 44 > > > > > > > --- a/grub-core/disk/cryptodisk.c > > > > > > > +++ b/grub-core/disk/cryptodisk.c > > > > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_= t disk) > > > > > > > =20 > > > > > > > #endif > > > > > > > =20 > > > > > > > -static int check_boot, have_it; > > > > > > > -static char *search_uuid; > > > > > > > - > > > > > > > static void > > > > > > > cryptodisk_close (grub_cryptodisk_t dev) > > > > > > > { > > > > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const= char > > > > > > > *name,=20 > > > > > > > FOR_CRYPTODISK_DEVS (cr) > > > > > > > { > > > > > > > - dev =3D cr->scan (source, search_uuid, check_boot); > > > > > > > + dev =3D cr->scan (source, cargs->search_uuid, cargs->che= ck_boot); > > > > > > > if (grub_errno) > > > > > > > return grub_errno; > > > > > > > if (!dev) > > > > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const= char > > > > > > > *name,=20 > > > > > > > grub_cryptodisk_insert (dev, name, source); > > > > > > > =20 > > > > > > > - have_it =3D 1; > > > > > > > + cargs->found_uuid =3D 1; > > > > > > > =20 > > > > > > > goto cleanup; > > > > > > > } > > > > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char > > > > > > > *sourcedev, const char *cheat)=20 > > > > > > > FOR_CRYPTODISK_DEVS (cr) > > > > > > > { > > > > > > > - dev =3D cr->scan (source, search_uuid, check_boot); > > > > > > > + dev =3D cr->scan (source, NULL, 0); > > > > > > > if (grub_errno) > > > > > > > return grub_errno; > > > > > > > if (!dev) > > > > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char= *name, > > > > > > > =20 > > > > > > > if (err) > > > > > > > grub_print_error (); > > > > > > > - return have_it && search_uuid ? 1 : 0; > > > > > > > + return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0; > > > > > > > } > > > > > > > =20 > > > > > > > static grub_err_t > > > > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_conte= xt_t > > > > > > > ctxt, int argc, char **args) cargs.key_len =3D > > > > > > > grub_strlen(state[3].arg); } > > > > > > > =20 > > > > > > > - have_it =3D 0; > > > > > > > if (state[0].set) /* uuid */ > > > > > > > { > > > > > > > grub_cryptodisk_t dev; > > > > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_con= text_t > > > > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE; > > > > > > > } > > > > > > > =20 > > > > > > > - check_boot =3D state[2].set; > > > > > > > - search_uuid =3D args[0]; > > > > > > > + cargs.check_boot =3D state[2].set; > > > > > > > + cargs.search_uuid =3D args[0]; > > > > > > > grub_device_iterate (&grub_cryptodisk_scan_device, &ca= rgs); > > > > > > > - search_uuid =3D NULL; > > > > > > > =20 > > > > > > > - if (!have_it) > > > > > > > + if (!cargs.found_uuid) > > > > > > > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such > > > > > > > cryptodisk found"); return GRUB_ERR_NONE; > > > > > > > } > > > > > > > else if (state[1].set || (argc =3D=3D 0 && state[2].set)) = /* -a|-b */ > > > > > > > { > > > > > > > - search_uuid =3D NULL; > > > > > > > - check_boot =3D state[2].set; > > > > > > > + cargs.check_boot =3D state[2].set; > > > > > > > grub_device_iterate (&grub_cryptodisk_scan_device, &ca= rgs); > > > > > > > - search_uuid =3D NULL; > > > > > > > return GRUB_ERR_NONE; > > > > > > > } > > > > > > > else > > > > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_conte= xt_t > > > > > > > ctxt, int argc, char **args) char *disklast =3D NULL; > > > > > > > grub_size_t len; > > > > > > > =20 > > > > > > > - search_uuid =3D NULL; > > > > > > > - check_boot =3D state[2].set; > > > > > > > + cargs.check_boot =3D state[2].set; > > > > > > > diskname =3D args[0]; > > > > > > > len =3D grub_strlen (diskname); > > > > > > > if (len && diskname[0] =3D=3D '(' && diskname[len - 1]= =3D=3D ')') > > > > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptod= isk.h > > > > > > > index 1070140d9..11062f43a 100644 > > > > > > > --- a/include/grub/cryptodisk.h > > > > > > > +++ b/include/grub/cryptodisk.h > > > > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t > > > > > > > =20 > > > > > > > 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 > > > > > > Aren't these parameters in a different scope than the key data?= These > > > > > > are only used for device discovery via `scan()`, while the othe= r ones > > > > > > are for decrypting the key. Do we want to split those up into t= wo > > > > > > different structs? > > > > >=20 > > > > > This struct is meant to be used for any data passed to the crypto > > > > > backend from cryptomount. All of those members are affected by > > > > > cryptomount options. So this struct isn't about anything in parti= cular, > > > > > just a common set of data passed to the crypto backends via > > > > > cryptomount. So I don't think two structs would improve anything = here. > > > > > Am I missing something? > > > > >=20 > > > > > Glenn > > > >=20 > > > > I'm mostly wondering about lifetimes of these parameters. They are = used > > > > in different phases of the cryptomount: some are used only at the t= ime > > > > of discovery, while others are used at decryption time, where it's = not > > > > immediately clear which parameters are used when without having a l= ook > > > > at the cryptodisk implementations. That's why I was thinking it mig= ht > > > > make more sense to split them up by those phases such that this bec= omes > > > > explicit. > > > >=20 > > > > Patrick > > >=20 > > > Okay, I think I see what you're saying. Essentially, as someone wanti= ng > > > to write a new crypto-backend, when I'm writing by scan function, how > > > do I know what fields in the cargs struct are valid (have been > > > assigned)? The answer would be to check if they are not NULL, and > > > otherwise they're available. I don't really see an issue. > > >=20 > > > Would your objection be alleviated by having cargs be redefined like = so: > > >=20 > > > struct grub_cryptomount_args > > > { > > > struct { > > > grub_uint32_t check_boot : 1; > > > char *search_uuid; > > > } scan; > > > struct { > > > grub_uint8_t *key_data; > > > grub_size_t key_len; > > > } recover_key; > > > struct { > > > ... > > > } common; > > > }; > > >=20 > > > Then in scan you know you can only use scan and common structs. I have > > > a common because the detached header changes will be such that scan a= nd > > > recover_key need to be provided with detached header. I think this w= ay > > > is more than is needed at the present moment. If I'm still not getting > > > it, can you be a little more concrete in what is problematic and how > > > you think it could be changed to be better? > > >=20 > > > Glenn > >=20 > > Sorry, took me quite some time to get to this. I like your proposed > > approach, and if we sprinkle in some comments about when those structs > > should be used then I think it would be easy enough to understand. It > > does raise the question whether we should instead define > > `grub_cryptomount_args_common` and then embed it in > > `grub_cryptomount_args_{scan,recover_key}` structs such that it is > > impossible to use args in the wrong phase (e.g. `recover_key` args > > during scan). But I feels a bit like bikeshedding, so please feel free > > to go with your preferred style. >=20 > I'm gathering that the issue with partitioning the cargs struct is > based on a concern about the proper use of the data. Perhaps the reason > I don't see the value in partitioning the struct is because I also > don't see how the data can be used in "the wrong phase". I'm guessing > this is mainly a concern with key_data and key_len and that they might > not be set during the scan phase. In that case key_len should be 0 and > key_data a NULL pointer. If the scan phase wants to use them, I don't > see why not, just make sure to check the values for validity first. Is > the concern that a crypto-backend author will try to "unlock" the > encrypted volume in scan? Perhaps I should add a couple of comments in > the grub_cryptodisk_dev struct briefly describing scan and recover_key > instead. It's rather about passing data that we know to be completely irrelevant to the function at this point in time, even if it's unset. But as I said above, this really isn't much of a strong concern, but rather feels like a small inconsistency in the API design to me. > I don't really like the nested struct solution I proposed > because then the backend has to use both the phase-specific child > struct and the common one, needing to remember which member is in which > struct. I don't like embedding the common members in two different > structs for, one for each phase, because then we have duplication of > data. Or is there a way I'm not thinking of which can define the > structs such that they share the common members? >=20 > What about a compromise, which would be to document in the member > comment which phase its intented to be used in? That's fine with me. I don't want to spend too much time discussing this point: in the end it won't really matter anyway given that we only got so many backends, and regardless of which way we go with, the end result is cleaner than what we've got right now. So again, please don't take my criticism on this point as a blocker. Patrick --PDwWNm7NLfMtO6Hw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmFin6AACgkQVbJhu7ck PpSrWg/+N/OfsGIDqbsJ9yQkLLDs0JJ/pzPHqumsx6a0z59mq8QV2o2HYlrZaGKJ RhJwyO0fJKLZgyC3RYfLA0Wjdq4b4jVKVvaAgWmxvEcS3d5JvKY+/BZFf3vKQSdv AZtu7Rs/mscKZxmsuFx268ik3lRRJb9BcLQQLVak7RaVayBOSArOSUmDbUw1PH7/ AdY+4GW0ejz53ngVwLXRMYw2WVkyEZwNpwvv3Og+On42okcA31W4otBBYsreZVKb Gc9n+0aEFSwl1z98erRZ/mSy+m5vo7brkQd3ZPtrsYELG3Np2cCRSSEPXgf9eyyr xhCqVt4lfHqfHJmjOhZ7Yke94XjHSlCy9pHUzomG7wL25jthX287Kfc8/Pworlhf r3QzpdSb6R+EiTx6XvyLlHeUuVzhAAcfTgHj4e1cI69bgvQIGwaEUU3VVFVPV0pd Ydu7ZF6uCWRt3ZIKs35vPxFmqFQOKfKrWMNSkNsoj5cVNckjnZWRgcZR+L7Mpqoa RwnNoBERkc2bZTQiakSL21IBv74wXepNSquAGBmRc6nSdYjRwt294vejnfHViwHZ WB4NvHldLn5AhjYMe7KpBmzig5+XecMbilBYbkR61lRQgof7+bdj5C1WPntQlbdh 4ChYK5WWBPtxP884jyseqAfLWzgiyL3OccNwrSWFZR624SM0cJI= =WxNk -----END PGP SIGNATURE----- --PDwWNm7NLfMtO6Hw--