From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1ko9NT-0003EC-SL for mharc-grub-devel@gnu.org; Sat, 12 Dec 2020 13:12:20 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:52278) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ko9NP-0003Af-G5 for grub-devel@gnu.org; Sat, 12 Dec 2020 13:12:15 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:52309) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1ko9NN-00087r-1I for grub-devel@gnu.org; Sat, 12 Dec 2020 13:12:15 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:37272 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2085649AbgLLPTV (ORCPT ); Sat, 12 Dec 2020 16:19:21 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Sat, 12 Dec 2020 16:19:18 +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: <20201212151918.4zr3rujy4opldypc@tomti.i.net-space.pl> References: <9fbd815f9360ed2faa45c386d4ed103103284417.1607466704.git.development@efficientek.com> <20201210160707.ansx6bpqxgjfj7vs@tomti.i.net-space.pl> <20201211191018.382b8264@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201211191018.382b8264@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.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 12 Dec 2020 18:12:15 -0000 On Fri, Dec 11, 2020 at 07:10:18PM -0600, Glenn Washburn wrote: > On Thu, 10 Dec 2020 17:07:07 +0100 > Daniel Kiper wrote: > > > 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. > > Actually, I wouldn't call this a clamp because in the overflow case > crypt->total_sectors always equals 0. I just realized this, and its > because grub_strtoull will return 2^64-1 thus causing the following > ALIGN_UP to overflow returning 0. Suffice to say that's not what we > want. My original intent was what happened before the ALIGN_UP code was > introduced, which would ALIGN_DOWN. Ugh... Right. That is why I still think you should do further calculations on value returned from grub_strtoull() if grub_errno == GRUB_ERR_NONE only. I understand that then crypt->total_sectors does not contain total sectors for a while but you can rectify this a bit by putting short comment before grub_strtoull() call. > Here's an example illustrating why I wanted and still think the intent > of this check is reasonable. Suppose we have a disk size 2^67 bytes > with 512 byte (2^9) sized sectors. It will have 2^58 sectors. Further > suppose, there is a LUKS volume with size 2^65 bytes starting at the > beginning and a sector size of 4096 bytes (2^12). This will cause an > overflow, so grub_strtoull will return 2^64-1, this should have us set > crypt->total_sectors to 2^52-1. Since we don't know how much the > overflow is (1 byte or 1 terabyte), we don't know how many more sectors > til the end of the LUKS encrypted area. In this case max_crypt_sectors > will be 2^(58+9-12) => 2^57 sectors. So here we see that > crypt->total_sectors < max_crypt_sectors in the overflow case. If we > do as you suggest crypt->total_sectors will be set to 2^57, and thus > it will be valid to read past the end of the encrypted data (ie. block > 2^56 of the 4k sector crypt will be a sector starting at byte 2^68, > which is more than the 2^65 byte size volume). > > On the one hand, I like your suggestion because it allows reading all > possible encrypted data, at the cost of reading, decrypting, and > returning non-encrypted data (ie random garbage). While keeping the > check, will prevent returning garbage at the cost of not allowing > access to all encrypted sectors. I think we should keep the check and > document a known limitation of 2^64 byte maximum sized LUKS volumes. > And that larger sized volumes can be read only up to byte 2^64. If think the code should look like this: /* * ...a comment saying what crypt->total_sectors contains * and why LUKS2 volumes larger than 2^64 does not work... */ crypt->total_sectors = grub_strtoull (segment.size, NULL, 10); if (grub_errno == GRUB_ERR_NONE) { crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 << crypt->log_sector_size); crypt->total_sectors >>= crypt->log_sector_size; } else if (grub_errno == GRUB_ERR_BAD_NUMBER) { ... continue; } if (grub_errno == GRUB_ERR_OUT_OF_RANGE || !crypt->total_sectors || crypt->total_sectors > max_crypt_sectors) { ... continue; /* * Yes, I think we should not guess crypt->total_sectors * value and always fail. It seems safer. */ } Daniel