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 ## 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 ## 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 ## 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 + Reviewed-by: Patrick Steinhardt ## 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 + Reviewed-by: Patrick Steinhardt ## grub-core/disk/cryptodisk.c ## @@ -- 2.28.0