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 --]
next prev 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox