[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cr
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk |
Date: |
Mon, 7 Dec 2020 22:28:53 -0600 |
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 <development@efficientek.com> 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 <development@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;
> +
> + 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)
> {
> - 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);
> + 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.json_slot_key,
> 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.json_slot_key,
> segment.size);
> + if (crypt->total_sectors > max_crypt_sectors)
> + crypt->total_sectors = max_crypt_sectors;
> + }
> + }
> +
> + if (crypt->total_sectors == 0)
> + {
> + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> has zero"
> + " sectors, skipping\n",
> + segment.json_slot_key);
> + 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.json_slot_key);
> + /* 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)); if (ret)
> {
> grub_dprintf ("luks2", "Decryption with keyslot
> \"%"PRIuGRUB_UINT64_T"\" failed: %s\n",
> - keyslot.slot_key, grub_errmsg);
> + keyslot.json_slot_key, grub_errmsg);
> continue;
> }
>
> @@ -638,7 +716,7 @@ luks2_recover_key (grub_disk_t source,
> if (ret)
> {
> grub_dprintf ("luks2", "Could not open keyslot
> \"%"PRIuGRUB_UINT64_T"\": %s\n",
> - keyslot.slot_key, grub_errmsg);
> + keyslot.json_slot_key, grub_errmsg);
> continue;
> }
>
> @@ -646,7 +724,7 @@ luks2_recover_key (grub_disk_t source,
> * TRANSLATORS: It's a cryptographic key slot: one element of
> an array
> * where each element is either empty or holds a key.
> */
> - grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"),
> keyslot.slot_key);
> + grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"),
> keyslot.json_slot_key);
> candidate_key_len = keyslot.key_size;
> break;
> 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 <grub/device.h>
> /* For NULL. */
> #include <grub/mm.h>
> +/* For ALIGN_UP. */
> +#include <grub/misc.h>
>
> /* 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));
> + return sector >> (log_sector_size - disk->log_sector_size);
> + }
> + else
> + return sector << (disk->log_sector_size - log_sector_size);
> +}
> +
> /* Convert to GRUB native disk sized sector from disk sized sector.
> */ static inline grub_disk_addr_t
> grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t
> sector)
- [PATCH v7 00/17] Cryptodisk fixes for v2.06 redux, Glenn Washburn, 2020/12/04
- [PATCH v7 11/17] cryptodisk: Properly handle non-512 byte sized sectors, Glenn Washburn, 2020/12/04
- [PATCH v7 15/17] mips: Enable __clzdi2(), Glenn Washburn, 2020/12/04
- [PATCH v7 04/17] luks2: Make sure all fields of output argument in luks2_parse_digest() are written to, Glenn Washburn, 2020/12/04
- [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals, Glenn Washburn, 2020/12/04