From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1knOTW-0005zh-TB for mharc-grub-devel@gnu.org; Thu, 10 Dec 2020 11:07:26 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:32886) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1knOTU-0005we-8W for grub-devel@gnu.org; Thu, 10 Dec 2020 11:07:24 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:48669) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1knOTR-0008Rh-Vi for grub-devel@gnu.org; Thu, 10 Dec 2020 11:07:23 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:60794 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S170607AbgLJQHQ (ORCPT ); Thu, 10 Dec 2020 17:07:16 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Thu, 10 Dec 2020 17:07:07 +0100 From: Daniel Kiper To: Glenn Washburn Cc: grub-devel@gnu.org, Patrick Steinhardt Subject: Re: [PATCH v8 14/18] luks2: Better error handling when setting up the cryptodisk Message-ID: <20201210160707.ansx6bpqxgjfj7vs@tomti.i.net-space.pl> References: <9fbd815f9360ed2faa45c386d4ed103103284417.1607466704.git.development@efficientek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9fbd815f9360ed2faa45c386d4ed103103284417.1607466704.git.development@efficientek.com> 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.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Dec 2020 16:07:24 -0000 On Tue, Dec 08, 2020 at 04:45:45PM -0600, Glenn Washburn wrote: > First, check to make sure that source disk has a known size. If not, print > debug message and return error. There are 4 cases where > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and uboot), and in > all those cases processing continues. So this is probably a bit > conservative. However, 3 of the cases seem pathological, and the other, > biosdisk, happens when booting from a cd. Since I doubt booting from a LUKS2 > volume on a cd is a big use case, we'll error until someone complains. > > Do some sanity checking on data coming from the luks header. If segment.size > is "dynamic", verify that the offset is not past the end of disk. Otherwise, > check for errors from grub_strtoull when converting segment size from > string. If a GRUB_ERR_BAD_NUMBER error was returned, then the string was > not a valid parsable number, so skip the key. If GRUB_ERR_OUT_OF_RANGE was > returned, then there was an overflow in converting to a 64-bit unsigned > integer. So this could be a very large disk (perhaps large raid array). > In this case, we want to continue to try to use this key, but only allow > access up to the end of the source disk. > > Signed-off-by: Glenn Washburn > --- > grub-core/disk/luks2.c | 84 ++++++++++++++++++++++++++++++++++++++++-- > include/grub/disk.h | 17 +++++++++ > 2 files changed, 98 insertions(+), 3 deletions(-) > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index 9abcb1c94..8cb11e899 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -600,12 +600,26 @@ luks2_recover_key (grub_disk_t source, > goto err; > } > > + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN) > + { > + /* FIXME: Allow use of source disk, and maybe cause errors in read. */ > + grub_dprintf ("luks2", "Source disk %s has an unknown size, " > + "conservatively returning error\n", source->name); > + ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2 source device"); > + goto err; > + } > + > /* Try all keyslot */ > for (json_idx = 0; json_idx < size; json_idx++) > { > + typeof(source->total_sectors) max_crypt_sectors = 0; > + > + grub_errno = GRUB_ERR_NONE; > ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, json_idx); > if (ret) > goto err; > + if (grub_errno != GRUB_ERR_NONE) > + grub_dprintf ("luks2", "Ignoring unhandled error %d from luks2_get_keyslot\n", grub_errno); > > if (keyslot.priority == 0) > { > @@ -619,11 +633,75 @@ luks2_recover_key (grub_disk_t source, > 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; > + /* Set to the source disk size, which is the maximum we allow. */ > + max_crypt_sectors = grub_disk_convert_sector(source, > + source->total_sectors, > + crypt->log_sector_size); > + > + if (max_crypt_sectors < crypt->offset_sectors) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has offset" > + " %"PRIuGRUB_UINT64_T" which is greater than" > + " source disk size %"PRIuGRUB_UINT64_T"," > + " skipping\n", > + segment.idx, crypt->offset_sectors, > + max_crypt_sectors); > + continue; > + } > + > 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; > + crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors; > else > - crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size; > + { > + grub_errno = GRUB_ERR_NONE; > + /* Convert segment.size to sectors, rounding up to nearest sector */ > + crypt->total_sectors = grub_strtoull (segment.size, NULL, 10); > + crypt->total_sectors = ALIGN_UP (crypt->total_sectors, > + 1 << crypt->log_sector_size); > + crypt->total_sectors >>= crypt->log_sector_size; > + > + if (grub_errno == GRUB_ERR_NONE) > + ; > + else if (grub_errno == GRUB_ERR_BAD_NUMBER) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size" > + " \"%s\" is not a parsable number\n", > + segment.idx, segment.size); > + 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. > + */ > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size" > + " %s overflowed 64-bit unsigned integer," > + " the end of the crypto device will be" > + " inaccessible\n", > + segment.idx, segment.size); > + if (crypt->total_sectors > max_crypt_sectors) I think this if is bogus. You should clamp crypt->total_sectors without any checks here. > + crypt->total_sectors = max_crypt_sectors; > + } > + } > + > + if (crypt->total_sectors == 0) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has zero" > + " sectors, skipping\n", > + segment.idx); > + continue; > + } > + else if (max_crypt_sectors < (crypt->offset_sectors + crypt->total_sectors)) > + { > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has last" > + " data position greater than source disk size," > + " the end of the crypto device will be" > + " inaccessible\n", > + segment.idx); > + /* Allow decryption up to the end of the source disk. */ > + crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors; > + } > > ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot, > (const grub_uint8_t *) passphrase, grub_strlen (passphrase)); > diff --git a/include/grub/disk.h b/include/grub/disk.h > index 0fb727d3d..f9227f285 100644 > --- a/include/grub/disk.h > +++ b/include/grub/disk.h > @@ -27,6 +27,8 @@ > #include > /* For NULL. */ > #include > +/* For ALIGN_UP. */ > +#include > > /* These are used to set a device id. When you add a new disk device, > you must define a new id for it here. */ > @@ -174,6 +176,21 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t; > /* Return value of grub_disk_native_sectors() in case disk size is unknown. */ > #define GRUB_DISK_SIZE_UNKNOWN 0xffffffffffffffffULL > > +/* Convert sector number from disk sized sectors to a log-size sized sector. */ > +static inline grub_disk_addr_t > +grub_disk_convert_sector (grub_disk_t disk, > + grub_disk_addr_t sector, > + grub_size_t log_sector_size) > +{ > + if (disk->log_sector_size < log_sector_size) > + { > + sector = ALIGN_UP (sector, 1 << (log_sector_size / disk->log_sector_size)); s#/#-#? > + return sector >> (log_sector_size - disk->log_sector_size); > + } > + else > + return sector << (disk->log_sector_size - log_sector_size); > +} Daniel