On Fri, 4 Dec 2020 14:19:29 +0100 Daniel Kiper <dki...@net-space.pl> wrote:
> On Thu, Dec 03, 2020 at 10:24:46AM -0600, Glenn Washburn wrote: > > On Thu, 3 Dec 2020 14:31:49 +0100 > > Daniel Kiper <dki...@net-space.pl> wrote: > > > > > On Fri, Nov 27, 2020 at 03:03:39AM -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", > > > > > > > > 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 so long as the source > > > > disk's size is greater than this segment size. Otherwise, this > > > > is an invalid segment, and continue on to the next key. > > > > > > > > Signed-off-by: Glenn Washburn <developm...@efficientek.com> > > > > --- > > > > grub-core/disk/luks2.c | 80 > > > > ++++++++++++++++++++++++++++++++++++++++-- include/grub/disk.h > > > > | 16 +++++++++ 2 files changed, 93 insertions(+), 3 > > > > deletions(-) > > > > > > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > > > > index b7ed0642e..01f9608e5 100644 > > > > --- a/grub-core/disk/luks2.c > > > > +++ b/grub-core/disk/luks2.c > > > > @@ -597,12 +597,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 (i = 0; i < size; i++) > > > > { > > > > + typeof(source->total_sectors) max_crypt_sectors = 0; > > > > + > > > > + grub_errno = GRUB_ERR_NONE; > > > > ret = luks2_get_keyslot (&keyslot, &digest, &segment, > > > > json, i); 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) > > > > { > > > > @@ -616,11 +630,71 @@ 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.slot_key, > > > > 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; > > > > + crypt->total_sectors = grub_strtoull (segment.size, > > > > NULL, 10) >> crypt->log_sector_size; > > > > > > I think ">>" should not happen here... > > > > Do you think this because ">>" should not operate on a value > > returned by a call to grub_strtoull that has errored? I don't > > think there's any problem because the return value is a number, so > > there's no harm in using ">>" on a garbage number to get another > > garbage number, as long as we don't use the value. (Its also not > > technically a garbage number, just one of two values in the error > > case) > > > > Also I didn't want to set total_sectors to grub_strtoull because it > > could be a little confusing since the return value of grub_strtoull > > is bytes not sectors. And yet storing in total_sectors. I could > > use a local variable, but was wanting to avoid that. But I'm fine > > with this suggestion. > > OK, let's leave it as it is... > > > > > + if (grub_errno == GRUB_ERR_NONE) > > > > + ; > > > > > > It should happen here... Or to be exact... > > > crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 << > > > crypt->log_sector_size); // Am I right? crypt->total_sectors >>= > > > crypt->log_sector_size; > > > > I don't think we need to decrypt partial blocks at the end. > > Cryptsetup/dm-crypt enforces disk sizes that are multiples of the > > sector size. So if the size is not a multiple of blocksize, > > there's a bug somewhere in cryptsetup/dm-crypt, and we don't need > > to deal with the partial block. At worse the use loses access to a > > partial block at the end that they should never have had access to > > in the first place. > > Does GRUB fail safely in such cases? I think the question boils down to the filesystem driver code. On second thought, I'll take the suggestion to round up to nearest sector. > > > > + else if(grub_errno == GRUB_ERR_BAD_NUMBER) > > > > > > Missing space before "("... > > > > Ack. > > > > > > + { > > > > + /* TODO: Unparsable number-string, try to use the > > > > whole disk */ > > > > + grub_dprintf ("luks2", "Segment > > > > \"%"PRIuGRUB_UINT64_T"\" size" > > > > + " \"%s\" is not a parsable > > > > number\n", > > > > + segment.slot_key, > > > > segment.size); > > > > > > crypt->total_sectors = max_crypt_sectors; ? > > > > Sure, I didn't think you'd want this since it potentially entails > > trying to read past the end of the luks data, and thus getting > > random data, which might crash (buggy) filesystem read code. > > OK but then I would drop "TODO" comment because we do not want to > make this case work... Ideally I think we do want to make this case work, which would entail being confident enough that the file system code won't horribly break. We may actually be at that point, I just don't know the various filesystem drivers well enough to know either way. If we had a setup to run a filesystem fuzzer over it that might help confidence levels. Since I think its pretty improbable that there's an unparsable segment.size but we can get all other needed params to unlock the drive, I'll drop the todo. Glenn _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel