[PATCH v2 0/9] Cryptodisk fixes for v2.06

[Patrick Steinhardt]

Hi,

this is the second version of cryptodisk fixes which I deem to be
important for the upcoming v2.06 release. Changes:

    - Patch 2: we're now zeroing out the UUID variable to avoid copying
               over uninitialized bytes. Thanks for spotting, Dennis!

    - Patch 3: I've replaced it with the updated version he's posted
               to address my feedback.

    - Patch 7: Updated by Glenn to now also fix writes.

I didn't yet get your test series to work, Glenn. I'll try again on
another day as I'm not on top of things today. Meanwhile, could you
please give it a go with this updated patch series?

Regards
Patrick

Glenn Washburn (6):
  luks2: Fix use of incorrect index and some error messages
  luks2: grub_cryptodisk_t->total_length is the max number of device
    native sectors
  cryptodisk: Unregister cryptomount command when removing module
  cryptodisk: Fix incorrect calculation of start sector
  cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'
  cryptodisk: Properly handle non-512 byte sized sectors

Patrick Steinhardt (3):
  json: Remove invalid typedef redefinition
  luks: Fix out-of-bounds copy of UUID
  luks2: Improve error reporting when decrypting/verifying key

 grub-core/disk/cryptodisk.c | 63 ++++++++++++++++++++-----------------
 grub-core/disk/luks.c       |  8 +++--
 grub-core/disk/luks2.c      | 54 +++++++++++++++++--------------
 grub-core/lib/json/json.h   |  9 +++---
 include/grub/cryptodisk.h   |  2 +-
 include/grub/disk.h         |  7 +++++
 6 files changed, 83 insertions(+), 60 deletions(-)

Range-diff against v1:
 1:  ee402de4d =  1:  ee402de4d json: Remove invalid typedef redefinition
 2:  8668b265f !  2:  5ecb9a4eb luks: Fix out-of-bounds copy of UUID
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/luks.c ##
    +@@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
    +       || grub_be_to_cpu16 (header.version) != 1)
    +     return NULL;
    + 
    ++  grub_memset (uuid, 0, sizeof (uuid));
    +   optr = uuid;
    +   for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
    +        iptr++)
     @@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char 
*check_uuid,
        newdev->source_disk = NULL;
        newdev->log_sector_size = 9;
 3:  7cf9d9526 !  3:  f8da5b4b4 luks2: Fix use of incorrect index and some 
error messages
    @@ Commit message
         Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/luks2.c ##
    -@@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
    +@@ grub-core/disk/luks2.c: luks2_parse_digest (grub_luks2_digest_t *out, 
const grub_json_t *digest)
    + 
    + static grub_err_t
    + luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, 
grub_luks2_segment_t *s,
    +-             const grub_json_t *root, grub_size_t i)
    ++             const grub_json_t *root, grub_size_t keyslot_idx)
    + {
    +   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
    +-  grub_size_t j, size;
    +-  grub_uint64_t idx;
    ++  grub_size_t i, size;
    ++  grub_uint64_t keyslot_key, digest_key, segment_key;
    + 
    +   /* Get nth keyslot */
    +   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
    +-      grub_json_getchild (&keyslot, &keyslots, i) ||
    +-      grub_json_getuint64 (&idx, &keyslot, NULL) ||
    ++      grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
    ++      grub_json_getuint64 (&keyslot_key, &keyslot, NULL) ||
    +       grub_json_getchild (&keyslot, &keyslot, 0) ||
    +       luks2_parse_keyslot (k, &keyslot))
    +-    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
%"PRIuGRUB_SIZE, i);
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
index %"PRIuGRUB_SIZE, keyslot_idx);
    + 
    +   /* Get digest that matches the keyslot. */
    +   if (grub_json_getvalue (&digests, root, "digests") ||
    +       grub_json_getsize (&size, &digests))
          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
    -   for (j = 0; j < size; j++)
    +-  for (j = 0; j < size; j++)
    ++  for (i = 0; i < size; i++)
          {
    --      if (grub_json_getchild (&digest, &digests, i) ||
    -+      if (grub_json_getchild (&digest, &digests, j) ||
    +       if (grub_json_getchild (&digest, &digests, i) ||
    ++    grub_json_getuint64 (&digest_key, &digest, NULL) ||
                grub_json_getchild (&digest, &digest, 0) ||
                luks2_parse_digest (d, &digest))
     -  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest 
%"PRIuGRUB_SIZE, i);
    -+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest 
%"PRIuGRUB_SIZE, j);
    ++  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index 
%"PRIuGRUB_SIZE, i);
      
    -       if ((d->keyslots & (1 << idx)))
    +-      if ((d->keyslots & (1 << idx)))
    ++      if ((d->keyslots & (1 << keyslot_key)))
        break;
          }
    -   if (j == size)
    +-  if (j == size)
     -      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot 
%"PRIuGRUB_SIZE);
    -+      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot 
%"PRIuGRUB_SIZE, i);
    ++  if (i == size)
    ++      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot 
\"%"PRIuGRUB_UINT64_T"\"", keyslot_key);
      
        /* Get segment that matches the digest. */
        if (grub_json_getvalue (&segments, root, "segments") ||
            grub_json_getsize (&size, &segments))
          return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
     -  for (j = 0; j < size; j++)
    -+  for (i = j, j = 0; j < size; j++)
    ++  for (i = 0; i < size; i++)
          {
    --      if (grub_json_getchild (&segment, &segments, i) ||
    -+      if (grub_json_getchild (&segment, &segments, j) ||
    -     grub_json_getuint64 (&idx, &segment, NULL) ||
    +       if (grub_json_getchild (&segment, &segments, i) ||
    +-    grub_json_getuint64 (&idx, &segment, NULL) ||
    ++    grub_json_getuint64 (&segment_key, &segment, NULL) ||
          grub_json_getchild (&segment, &segment, 0) ||
                luks2_parse_segment (s, &segment))
     -  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, i);
    -+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, j);
    ++  return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
index %"PRIuGRUB_SIZE, i);
      
    -       if ((d->segments & (1 << idx)))
    +-      if ((d->segments & (1 << idx)))
    ++      if ((d->segments & (1 << segment_key)))
        break;
          }
    -   if (j == size)
    +-  if (j == size)
     -    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest 
%"PRIuGRUB_SIZE);
    -+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest 
%"PRIuGRUB_SIZE, i);
    ++  if (i == size)
    ++    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest 
\"%"PRIuGRUB_UINT64_T"\"", digest_key);
      
        return GRUB_ERR_NONE;
      }
 4:  c3bf40e31 =  4:  150491a07 luks2: grub_cryptodisk_t->total_length is the 
max number of device native sectors
 5:  4b820da6c =  5:  7dbfad568 luks2: Improve error reporting when 
decrypting/verifying key
 6:  f7176a87e =  6:  dbf25a0ae cryptodisk: Unregister cryptomount command when 
removing module
 7:  13d0720d6 !  7:  4ee7f8774 cryptodisk: Incorrect calculation of start 
sector for grub_disk_read in grub_cryptodisk_read
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    cryptodisk: Incorrect calculation of start sector for grub_disk_read 
in grub_cryptodisk_read
    +    cryptodisk: Fix incorrect calculation of start sector
     
         Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors 
of size
         native to the cryptodisk device. The sector is correctly transformed 
into
    @@ Commit message
         transformed. It would be nice if the type system would help us with 
this.
     
         Signed-off-by: Glenn Washburn <development@efficientek.com>
    +    Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk, 
grub_disk_addr_t sector,
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_read (grub_disk_t disk, 
grub_disk_a
        err = grub_disk_read (dev->source_disk,
     -                  (sector << (disk->log_sector_size
     -                             - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
    -+                  ((sector + dev->offset) << (disk->log_sector_size
    -+                                             - GRUB_DISK_SECTOR_BITS)), 0,
    -                   size << disk->log_sector_size, buf);
    +-                  size << disk->log_sector_size, buf);
    ++                  grub_disk_from_native_sector (disk, sector + 
dev->offset),
    ++                  0, size << disk->log_sector_size, buf);
        if (err)
          {
    +       grub_dprintf ("cryptodisk", "grub_disk_read failed with error 
%d\n", err);
    +@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_write (grub_disk_t disk, 
grub_disk_addr_t sector,
    +     }
    + 
    +   /* Since ->write was called so disk.mod is loaded but be paranoid  */
    +-  
    ++  sector = sector + dev->offset;
    +   if (grub_disk_write_weak)
    +     err = grub_disk_write_weak (dev->source_disk,
    +-                          (sector << (disk->log_sector_size
    +-                                      - GRUB_DISK_SECTOR_BITS))
    +-                          + dev->offset,
    ++                          grub_disk_from_native_sector (disk, sector),
    +                           0, size << disk->log_sector_size, tmp);
    +   else
    +     err = grub_error (GRUB_ERR_BUG, "disk.mod not loaded");
    +
    + ## include/grub/disk.h ##
    +@@ include/grub/disk.h: typedef struct grub_disk_memberlist 
*grub_disk_memberlist_t;
    + /* Return value of grub_disk_get_size() in case disk size is unknown. */
    + #define GRUB_DISK_SIZE_UNKNOWN     0xffffffffffffffffULL
    + 
    ++/* 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)
    ++{
    ++  return sector << (disk->log_sector_size - GRUB_DISK_SECTOR_BITS);
    ++}
    ++
    + /* This is called from the memory manager.  */
    + void grub_disk_cache_invalidate_all (void);
    + 
 8:  12bc26698 =  8:  4aecb174b cryptodisk: Fix cipher IV mode 'plain64' always 
being set as 'plain'
 9:  32b463e00 !  9:  f520aecfa cryptodisk: Properly handle non-512 byte sized 
sectors
    @@ Commit message
         aestetically pleasing than desired.
     
         Signed-off-by: Glenn Washburn <development@efficientek.com>
    +    Reviewed-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/disk/cryptodisk.c ##
     @@
-- 
2.28.0

signature.asc
Description: PGP signature