All of lore.kernel.org
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: grub-devel@gnu.org
Cc: Denis GNUtoo Carikli <GNUtoo@cyberdimension.org>,
	Glenn Washburn <development@efficientek.com>,
	Daniel Kiper <daniel.kiper@oracle.com>
Subject: [PATCH v2 0/9] Cryptodisk fixes for v2.06
Date: Wed, 26 Aug 2020 10:13:18 +0200	[thread overview]
Message-ID: <cover.1598429170.git.ps@pks.im> (raw)
In-Reply-To: <cover.1598179677.git.ps@pks.im>

[-- Attachment #1: Type: text/plain, Size: 11136 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply	other threads:[~2020-08-26  8:11 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-23 10:59 [PATCH 0/9] Cryptodisk fixes for v2.06 Patrick Steinhardt
2020-08-23 10:59 ` [PATCH 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-08-23 10:59 ` [PATCH 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-08-23 21:34   ` Denis 'GNUtoo' Carikli
2020-08-26  7:18     ` Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-08-24  6:30   ` Glenn Washburn
2020-08-24  6:33     ` Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-08-23 11:03 ` [PATCH 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 7/9] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-08-23 11:04 ` [PATCH 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-08-24  6:22 ` [PATCH 0/9] Cryptodisk fixes for v2.06 Glenn Washburn
2020-08-24  6:31   ` Patrick Steinhardt
2020-08-26  8:13 ` Patrick Steinhardt [this message]
2020-08-26  8:13   ` [PATCH v2 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-08-26 23:44     ` [PATCH] cryptodisk: Incorrect calculation of sector in grub_cryptodisk_read/write Glenn Washburn
2020-08-26 23:50       ` Glenn Washburn
2020-08-28  7:12         ` Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 7/9] cryptodisk: Fix incorrect calculation of start sector Patrick Steinhardt
2020-08-26  8:13   ` [PATCH v2 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-08-26  8:14   ` [PATCH v2 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-08-31 18:43     ` Glenn Washburn
2020-09-01 15:28       ` Patrick Steinhardt
2020-09-01 23:21     ` [PATCH] " Glenn Washburn
2020-09-02  0:01       ` Glenn Washburn
2020-09-07 15:28         ` Patrick Steinhardt
2020-08-26 22:16   ` [PATCH v2 0/9] Cryptodisk fixes for v2.06 Glenn Washburn
2020-08-28  7:17     ` Patrick Steinhardt
2020-09-07 15:27 ` [PATCH v3 " Patrick Steinhardt
2020-09-07 15:27   ` [PATCH v3 1/9] json: Remove invalid typedef redefinition Patrick Steinhardt
2020-09-07 15:27   ` [PATCH v3 2/9] luks: Fix out-of-bounds copy of UUID Patrick Steinhardt
2020-09-07 15:27   ` [PATCH v3 3/9] luks2: Fix use of incorrect index and some error messages Patrick Steinhardt
2020-09-08 12:58     ` Daniel Kiper
2020-09-21  6:45       ` Glenn Washburn
2020-09-21 11:24         ` Daniel Kiper
2020-09-07 15:27   ` [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors Patrick Steinhardt
2020-09-08 13:21     ` Daniel Kiper
2020-09-21  6:28       ` Glenn Washburn
2020-09-21 11:23         ` Daniel Kiper
2020-10-03  5:42           ` Glenn Washburn
2020-10-27 19:11             ` Daniel Kiper
2020-10-29 19:53               ` Glenn Washburn
2020-10-30 12:49                 ` Daniel Kiper
2020-11-03 20:21                   ` Glenn Washburn
2020-11-04 13:15                     ` Daniel Kiper
2020-11-06  6:41                       ` Glenn Washburn
2020-09-07 15:27   ` [PATCH v3 5/9] luks2: Improve error reporting when decrypting/verifying key Patrick Steinhardt
2020-09-07 15:27   ` [PATCH v3 6/9] cryptodisk: Unregister cryptomount command when removing module Patrick Steinhardt
2020-09-08 13:28     ` Daniel Kiper
2020-09-21  6:45       ` Glenn Washburn
2020-09-21 11:25         ` Daniel Kiper
2020-09-07 15:27   ` [PATCH v3 7/9] cryptodisk: Fix incorrect calculation of start sector Patrick Steinhardt
2020-09-07 15:28   ` [PATCH v3 8/9] cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain' Patrick Steinhardt
2020-09-08 13:42     ` Daniel Kiper
2020-09-07 15:28   ` [PATCH v3 9/9] cryptodisk: Properly handle non-512 byte sized sectors Patrick Steinhardt
2020-09-09 11:21     ` Daniel Kiper
2020-09-21  5:58       ` Glenn Washburn
2020-09-21 11:16         ` Daniel Kiper
2020-09-09 11:28   ` [PATCH v3 0/9] Cryptodisk fixes for v2.06 Daniel Kiper
2020-09-17 14:14   ` Patrick Steinhardt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1598429170.git.ps@pks.im \
    --to=ps@pks.im \
    --cc=GNUtoo@cyberdimension.org \
    --cc=daniel.kiper@oracle.com \
    --cc=development@efficientek.com \
    --cc=grub-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.