From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1p0pOC-0004f4-1v for mharc-grub-devel@gnu.org; Thu, 01 Dec 2022 14:38:32 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0pO8-0004en-BM for grub-devel@gnu.org; Thu, 01 Dec 2022 14:38:30 -0500 Received: from dibed.net-space.pl ([84.10.22.86]) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1p0pO3-0006t8-El for grub-devel@gnu.org; Thu, 01 Dec 2022 14:38:26 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:48996 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2162454AbiLATiE (ORCPT ); Thu, 1 Dec 2022 20:38:04 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 1 Dec 2022 20:38:02 +0100 From: Daniel Kiper To: Glenn Washburn Cc: Maxim Fomin , "grub-devel@gnu.org" , "ps@pks.im" Subject: Re: [PATCH v8 1/1] plainmount: Support plain encryption mode Message-ID: <20221201193802.e5y65f2f5utzhmf3@tomti.i.net-space.pl> References: <20221129171311.k5yrlmmt4c4bo4r5@tomti.i.net-space.pl> <20221130235833.7bf20dc5@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221130235833.7bf20dc5@crass-HP-ZBook-15-G2> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=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.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Dec 2022 19:38:30 -0000 On Wed, Nov 30, 2022 at 11:58:33PM -0600, Glenn Washburn wrote: > On Tue, 29 Nov 2022 18:13:11 +0100 > Daniel Kiper wrote: > > > First of all, sorry for late reply. I hope I will be able to review > > next version of this patch much faster. > > > > On Sat, Oct 29, 2022 at 05:40:42PM +0000, Maxim Fomin wrote: > > > From 2b1d2deb3f2416cbc3e7d25cbc4141a3078eaf68 Mon Sep 17 00:00:00 > > > 2001 From: Maxim Fomin > > > Date: Sat, 29 Oct 2022 18:18:58 +0100 > > > Subject: [PATCH v8 1/1] plainmount: Support plain encryption mode > > > > > > This patch adds support for plain encryption mode (plain dm-crypt) > > > via new module/command named 'plainmount'. > > > > > > Signed-off-by: Maxim Fomin > > > > Please put "---" behind SOB and before "Difference with..." next > > time... > > [...] > > > > + > > > +/* Configure cryptodisk uuid */ > > > +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char > > > *user_uuid) +{ > > > + grub_size_t pos = 0; > > > + > > > + /* Size of user_uuid is checked in main func */ > > > + if (user_uuid != NULL) > > > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid)); > > > > I think grub_strcpy() instead of grub_memcpy() would be more natural > > in string contexts. And then you do not need grub_strlen(). > > > > Is it safe to assume here that dev->uuid has been zeroed earlier? > > Yes, its zalloc'd in grub_cmd_plainmount below. OK, cool! [...] > > > + /* Check uuid length */ > > > + if (state[OPTION_UUID].set && grub_strlen > > > (state[OPTION_UUID].arg) > > > > + sizeof (PLAINMOUNT_DEFAULT_UUID)) > > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > > + N_("specified UUID exceeds maximum size")); > > > > What will happen if somebody passes shorter, probably non-valid, UUID? > > I think we should check UUID is not shorter than something too. > > Afaik, we don't really care what the UUID string is, just that its > unique. It doesn't have to be a real UUID either. A minimum length > check of 1 might be good, otherwise '--uuid ""' will create an empty > uuid, but its not a big deal either. We might think about a character > validation check so that characters like ')' can't be in the string. > None of this will cause real problems as far as I can tell, but some > features may not work right. For example, having ')' in the uuid will > make it so that you can't access the crypto device via the > '(cruuid/)' syntax. I also don't mind not doing the validation > and letting the user shoot themselves in the foot if they so choose. I think we should check UUID size and probably it should not be smaller than 1 and larger than "sizeof(PLAINMOUNT_DEFAULT_UUID) - 1". I do not think we should check validity/content of the UUID. Daniel