From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kf1w4-000202-Df for mharc-grub-devel@gnu.org; Tue, 17 Nov 2020 09:26:20 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:35020) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kf1w2-0001zu-N9 for grub-devel@gnu.org; Tue, 17 Nov 2020 09:26:18 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:60028) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1kf1vz-0006oi-P4 for grub-devel@gnu.org; Tue, 17 Nov 2020 09:26:18 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:49654 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S153164AbgKQO0K (ORCPT ); Tue, 17 Nov 2020 15:26:10 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Tue, 17 Nov 2020 15:26:08 +0100 From: Daniel Kiper To: Glenn Washburn Cc: grub-devel@gnu.org, Patrick Steinhardt Subject: Re: [PATCH v4 14/15] luks2: Better error handling when setting up the cryptodisk. Message-ID: <20201117142608.56ubfsgrfxlkkekg@tomti.i.net-space.pl> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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-detected-operating-system: by eggs.gnu.org: First seen = 2020/11/17 08:47:46 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] 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.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Nov 2020 14:26:18 -0000 On Fri, Nov 06, 2020 at 10:44:34PM -0600, Glenn Washburn wrote: > Signed-off-by: Glenn Washburn > --- > grub-core/disk/luks2.c | 70 +++++++++++++++++++++++++++++++++++++++--- > include/grub/misc.h | 2 ++ > 2 files changed, 67 insertions(+), 5 deletions(-) > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index 4a4a0dec4..751b48d6a 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -600,9 +600,16 @@ luks2_recover_key (grub_disk_t source, > goto err; > } > > + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN) > + { > + ret = grub_error (GRUB_ERR_BUG, "not a luks2 device"); > + goto err; > + } > + > /* Try all keyslot */ > for (i = 0; i < size; i++) > { > + grub_errno = GRUB_ERR_NONE; > ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, i); > if (ret) > goto err; > @@ -617,13 +624,66 @@ luks2_recover_key (grub_disk_t source, > > /* Set up disk according to keyslot's segment. */ > crypt->offset_sectors = grub_divmod64 (segment.offset, segment.sector_size, NULL); > - crypt->log_sector_size = sizeof (unsigned int) * 8 > - - __builtin_clz ((unsigned int) segment.sector_size) - 1; > + crypt->log_sector_size = grub_log2ull (segment.sector_size); This change does not belong to this patch. > if (grub_strcmp (segment.size, "dynamic") == 0) > - crypt->total_sectors = (grub_disk_get_size (source) >> (crypt->log_sector_size - source->log_sector_size)) > - - crypt->offset_sectors; > + { > + /* Convert source sized number of sectors to cryptodisk sized sectors */ > + crypt->total_sectors = source->total_sectors >> (crypt->log_sector_size - source->log_sector_size); If you add the other checks could you also add the following check: crypt->log_sector_size >= source->log_sector_size ? > + if (crypt->total_sectors < crypt->offset_sectors) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" offset" > + " is greater than disk size.", Please replace "." at the end of the message with "\n" here and below... > + segment.slot_key); > + continue; > + } > + > + crypt->total_sectors -= crypt->offset_sectors; > + } > else > - crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size; > + { > + crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size; > + if (grub_errno == GRUB_ERR_NONE) > + ; > + else if(grub_errno == GRUB_ERR_BAD_NUMBER) > + { > + /* TODO: Unparsable number-string, try to use the whole disk */ > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size" > + " is not a parsable number.", > + segment.slot_key); > + continue; > + } > + else if(grub_errno == GRUB_ERR_OUT_OF_RANGE) > + { > + /* There was an overflow in parsing segment.size, so disk must > + * be very large or the string is incorrect. */ Please format this comment properly. > + if ((source->total_sectors > + >> (crypt->log_sector_size - source->log_sector_size)) > + > crypt->total_sectors) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"" > + " size is very large. The end may be" > + " inaccessible.", > + segment.slot_key); > + } > + else > + { > + /* FIXME: Set total_sectors as in "dynamic" case. */ > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"" > + " size greater than the source" > + " device.", > + segment.slot_key); > + continue; > + } > + } > + } > + > + if (crypt->total_sectors == 0) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has" > + " zero sectors, skipping.", > + segment.slot_key); > + continue; > + } > > ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot, > (const grub_uint8_t *) passphrase, grub_strlen (passphrase)); > diff --git a/include/grub/misc.h b/include/grub/misc.h > index b7ca6dd58..ec25131ba 100644 > --- a/include/grub/misc.h > +++ b/include/grub/misc.h > @@ -481,5 +481,7 @@ void EXPORT_FUNC(grub_real_boot_time) (const char *file, > > #define grub_max(a, b) (((a) > (b)) ? (a) : (b)) > #define grub_min(a, b) (((a) < (b)) ? (a) : (b)) Please add empty line here. > +#define grub_log2ull(n) (GRUB_TYPE_BITS (grub_uint64_t) \ > + - __builtin_clzll (n) - 1) This change does not belong to this patch too. Additionally, please double check that misc.h includes all headers which define GRUB_TYPE_BITS(), grub_uint64_t and __builtin_clzll(). By the way, may I ask you to stop adding full stop to the end of email subject? Daniel