From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mXJlG-00078O-CC for mharc-grub-devel@gnu.org; Mon, 04 Oct 2021 04:55:50 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:57000) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXJl4-000755-Uy for grub-devel@gnu.org; Mon, 04 Oct 2021 04:55:40 -0400 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:47383) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXJl1-00081z-5S for grub-devel@gnu.org; Mon, 04 Oct 2021 04:55:37 -0400 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.west.internal (Postfix) with ESMTP id D37533200D78; Mon, 4 Oct 2021 04:55:31 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Mon, 04 Oct 2021 04:55:32 -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=hH9QJZg7CMS2xR5SCt0XpmQju8i po7/CIqD72u0dlfY=; b=kWm4ZO50qOFAobfQn6GkKmEk5hbbPBwyPU2zDaHH9IN MM8PdAf6P7ZYdyVhcIBUAcwar2gpf+zuy8UQ5OYmIm9qWlCyczEN6fC5Fn8yiZ3d Eggatl9dCQw5uum7mrc2GjLbOi+umL5Ks93kfBl+ARfcHKpY7fbFjYdRCvSb5VJE SedXHQZkl5nwk3xXdfsF051WR0KPl/5V73/7Ohub6iRlqUMz6IckhNIzFbVdvQTv PCM1mz12iVxyswWyl50OChC0ZSrrwlW0QB9dVShIBJD2A71p/nSeHNZzBG/44WhS psnobzkSkr0K9yWJ7WyZ/KMycgj+2aSNAqKQJID3iBQ== 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=hH9QJZ g7CMS2xR5SCt0XpmQju8ipo7/CIqD72u0dlfY=; b=glyYAeo0reQmhO0U0rCnRt /HrjNdwpUYUH9t+3lU4+b4qeTuRlUNu0TcxESxYVJ2w2RAgeI/eGGlQ07yoXqexo 3Ktiq1E3NplD1z2yzGn+VYBjjJqW3I+ptbb2TTGFnVZeZmlpWcY1KU9QgLIamJNZ NFI9cgTzdARIqI4yqgvxRyrnvi3UD7ZxQ2YuHpkMEUMJol6VSC6nUpjW3iia6Fjq iDqA+MfO5qMhRKcIr20rVZqeDxS/J4bm3CGy7hPcIfBTSZ6CCxt67LRzJNjYMGKy MhW6xoqydFSFe69XoAgcromdDUJ3bas9zNdO1bLRW8klToEvXXbR5huX6z7sL4tA == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudelvddgtdekucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 4 Oct 2021 04:55:30 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 2e113953 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Mon, 4 Oct 2021 08:55:29 +0000 (UTC) Date: Mon, 4 Oct 2021 10:55:21 +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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="8zXmxBX8peIYyros" Content-Disposition: inline In-Reply-To: <20210913210515.5753cf48@ubuntu> Received-SPF: pass client-ip=64.147.123.19; envelope-from=ps@pks.im; helo=wout3-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: Mon, 04 Oct 2021 08:55:41 -0000 --8zXmxBX8peIYyros Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 100644 > > > > > --- a/grub-core/disk/cryptodisk.c > > > > > +++ b/grub-core/disk/cryptodisk.c > > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t di= sk) > > > > > =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->check_b= oot); > > > > > 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 *na= me, > > > > > =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_context_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_context= _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, &cargs); > > > > > - 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, &cargs); > > > > > - search_uuid =3D NULL; > > > > > return GRUB_ERR_NONE; > > > > > } > > > > > else > > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_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/cryptodisk.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? The= se > > > > are only used for device discovery via `scan()`, while the other on= es > > > > are for decrypting the key. Do we want to split those up into two > > > > 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 particula= r, > > > 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 time > > of discovery, while others are used at decryption time, where it's not > > immediately clear which parameters are used when without having a look > > at the cryptodisk implementations. That's why I was thinking it might > > make more sense to split them up by those phases such that this becomes > > explicit. > >=20 > > Patrick >=20 > Okay, I think I see what you're saying. Essentially, as someone wanting > 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 and > recover_key need to be provided with detached header. I think this way > 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 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. Patrick --8zXmxBX8peIYyros Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmFawXgACgkQVbJhu7ck PpSzhhAAq7Fd1I6yhMQ0z2tR9hU/hJt7TAGIwlciIlajCqbLj9BA4IYH+PcnKUUn bLRoc5Eu979jouEfYWsNHJWImvd4+2t0PVCAQfwnEg0+nI82PqUpezc+8lkj1ghb woycFNJ36qK6SmOAFkioMQVEHAd+owJ7m51vkMGpDLlPRNB37nI0zGjwG8FlbbsO qSNMYrWj6k+ZddLXiGEWqKx1BKCIYc/r1LeeqlH/vs5OWWk1BFeD+oBIALCdexbY Nwu7QRiN8P2TSfbYnWpT0SwxMaxtc+FCV8ldeQ9Q0uB+2I4QMqa0rkiMpTgHHZWS 6UsRFi1ImRcM9U2eFbNR7ymqD+8QBOTydzdRmVwAjtRq3VwWnZmVaMr9fjlrCMmh BJ5rIfjNPUCKjWs2cojG3GQmjjJNzRLIJF/uykPpY3FoZd49qCDbB2cbBUILOa1Q 1fm+s8vKOZQFsSjQxMD9t7vdpzDjj4LQhgoNsary0DhXK1a8SMRDbOTGFqndnOiJ ZNJE9Hwl6cwaE30X/DiGlzbeswRgRzOBfriI9l9IIyQ+ep3z7SNCVtI+jc493dNx sF+5a3olreSRHcoSefg9jUsYL8DlvnkoRXF/mGunGvwOVsHCqRCvNgUGTBWKh+4K CQPl3KOMokR4R5tyYEyBR+jHPr3zkmXfmqT6xTVWIDKVa2FC9/M= =Mtcf -----END PGP SIGNATURE----- --8zXmxBX8peIYyros--