On Mon, Dec 07, 2020 at 10:04:01PM -0600, Glenn Washburn wrote: > On Sun, 6 Dec 2020 20:35:13 +0100 > Patrick Steinhardt <p...@pks.im> wrote: > > > On Fri, Dec 04, 2020 at 10:43:41AM -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", > > > > Nit: there's something missing here. > > Yep, thanks for catching that. I was going to complete this and forgot > in my rush to get the series out before some travel. I'll rework that. > > > > 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 | 98 > > > +++++++++++++++++++++++++++++++++++++----- include/grub/disk.h | > > > 17 ++++++++ 2 files changed, 105 insertions(+), 10 deletions(-) > > > > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > > > index de2e70796..1bb3a333d 100644 > > > --- a/grub-core/disk/luks2.c > > > +++ b/grub-core/disk/luks2.c > > > @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, > > > grub_luks2_digest_t *d, grub_luks2_s break; > > > } > > > if (i == size) > > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for > > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key); > > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for > > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key); > > > /* Get segment that matches the digest. */ > > > if (grub_json_getvalue (&segments, root, "segments") || > > > @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, > > > grub_luks2_digest_t *d, grub_luks2_s break; > > > } > > > if (i == size) > > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for > > > digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key); > > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for > > > digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key); > > > return GRUB_ERR_NONE; > > > } > > > @@ -600,37 +600,115 @@ 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; > > > > Please bear with me if this has been discussed in a previous round, > > but why exactly do we cast `max_crypt_sectors` to the type of > > `source->total_sectors`? > > Technically, this isn't a cast. Its a variable declaration. But I'm > using the typeof(source->total_sectors) because max_crypt_sectors can > be no more or less than the total sectors, ie its of the same type.
Oh, of course. I somehow didn't realize this at all, probably because this is not something one sees that often. > > And isn't the variable always set anyway in > > case the keyslot has a non-zero priority? > > Yep. Are you suggesting that it need not be initialized? This is true, > but I don't think that's a problem. I think generally initializing > things is a good practice. It might be problematic if this was in an > oft used function (or it might not, would need to see if the compiler > was smart enough to ignore the initialization). But that > initialization is going to happen very rarely in the lifetime of a grub > execution instance. I also don't really care either way. > > Glenn I just didn't realize it's a declaration, so keeping the initialization is fine. Patrick
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel