Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
On Mon, Dec 07, 2020 at 10:04:01PM -0600, Glenn Washburn wrote:
> On Sun, 6 Dec 2020 20:35:13 +0100
> Patrick Steinhardt 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
> > > ---
> > > 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 (, 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
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
This patch got mangled accidentally with patch 5. I'll update them
both.
Glenn
On Fri, 4 Dec 2020 10:43:41 -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
> ---
> 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 (, 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;
> +
> + grub_errno = GRUB_ERR_NONE;
>ret = luks2_get_keyslot (, , , 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)
> {
> - grub_dprintf ("luks2", "Ignoring keyslot
> \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.slot_key);
> + grub_dprintf ("luks2", "Ignoring keyslot
> \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.json_slot_key);
> continue; }
>
> - grub_dprintf ("luks2", "Trying keyslot
> \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.slot_key);
> + grub_dprintf ("luks2", "Trying keyslot
> \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.json_slot_key);
>/* 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;
> + /* 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.json_slot_key,
> crypt->offset_sectors,
> + max_crypt_sectors);
> +
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
On Sun, 6 Dec 2020 20:35:13 +0100
Patrick Steinhardt 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
> > ---
> > 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 (, 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.
> 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
> Patrick
>
> > +
> > + grub_errno = GRUB_ERR_NONE;
> >ret = luks2_get_keyslot (, , , 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)
> > {
> > - grub_dprintf ("luks2", "Ignoring keyslot
> > \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.slot_key);
> > + grub_dprintf ("luks2", "Ignoring
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
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.
> 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
> ---
> 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 (, 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`? And isn't the variable always set anyway in
case the keyslot has a non-zero priority?
Patrick
> +
> + grub_errno = GRUB_ERR_NONE;
>ret = luks2_get_keyslot (, , , 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)
> {
> - grub_dprintf ("luks2", "Ignoring keyslot \"%"PRIuGRUB_UINT64_T"\" due
> to priority\n", keyslot.slot_key);
> + grub_dprintf ("luks2", "Ignoring keyslot \"%"PRIuGRUB_UINT64_T"\" due
> to priority\n", keyslot.json_slot_key);
> continue;
> }
>
> - grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n",
> keyslot.slot_key);
> + grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n",
> keyslot.json_slot_key);
>
>/* 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;
> + /* 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
