Re: [PATCH v2 00/10] Cryptodisk fixes for v2.06 redux

[Patrick Steinhardt]

On Sat, Oct 03, 2020 at 05:55:24PM -0500, Glenn Washburn wrote:
> This is a minor update to fix patch 3, where I missed updating the format 
> string
> type code. This was causing i386 builds to fail. Rangediff is included.

Cool, thanks a lot for taking care of this! I've left a few comments
here and there, but overall things look good to me.

Patrick

> Glenn Washburn (10):
>   luks2: Fix use of incorrect index and some grub_error() messages.
>   luks2: Improve readability in luks2_get_keyslot.
>   luks2: Use more intuitive keyslot key instead of index when naming
>     keyslot.
>   luks2: grub_cryptodisk_t->total_length is the max number of device
>     native sectors
>   cryptodisk: Fix cipher IV mode 'plain64' always being set as 'plain'.
>   cryptodisk: Properly handle non-512 byte sized sectors.
>   cryptodisk: Replace some literals with constants in
>     grub_cryptodisk_endecrypt.
>   cryptodisk: Rename total_length field in grub_cryptodisk_t to
>     total_sectors.
>   cryptodisk: Rename offset in grub_cryptodisk_t to offset_sectors.
>   luks2: Rename source disk variabled named 'disk' to 'source' as in
>     luks.c.
> 
>  grub-core/disk/cryptodisk.c | 78 ++++++++++++++++++--------------
>  grub-core/disk/geli.c       |  4 +-
>  grub-core/disk/luks.c       |  9 ++--
>  grub-core/disk/luks2.c      | 88 ++++++++++++++++++++-----------------
>  include/grub/cryptodisk.h   | 18 ++++++--
>  include/grub/types.h        |  3 ++
>  6 files changed, 118 insertions(+), 82 deletions(-)
> 
> Range-diff against v1:
> 1:  00646c92f ! 1:  e2433b8ab luks2: Use more intuitive keyslot key instead 
> of index when naming keyslot.
>     @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
>             if (keyslot.priority == 0)
>               {
>      -          grub_dprintf ("luks2", "Ignoring keyslot %"PRIuGRUB_SIZE" due 
> to priority\n", i);
>     -+          grub_dprintf ("luks2", "Ignoring keyslot %"PRIuGRUB_SIZE" due 
> to priority\n", keyslot_key);
>     ++          grub_dprintf ("luks2", "Ignoring keyslot %"PRIuGRUB_UINT64_T" 
> due to priority\n", keyslot_key);
>                 continue;
>               }
>       
>      -      grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
>     -+      grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", 
> keyslot_key);
>     ++      grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_UINT64_T"\n", 
> keyslot_key);
>       
>             /* Set up disk according to keyslot's segment. */
>             crypt->offset = grub_divmod64 (segment.offset, 
> segment.sector_size, NULL);
>      @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
>     +                                (const grub_uint8_t *) passphrase, 
> grub_strlen (passphrase));
>             if (ret)
>               {
>     -           grub_dprintf ("luks2", "Decryption with keyslot 
> %"PRIuGRUB_SIZE" failed: %s\n",
>     +-          grub_dprintf ("luks2", "Decryption with keyslot 
> %"PRIuGRUB_SIZE" failed: %s\n",
>      -                        i, grub_errmsg);
>     ++          grub_dprintf ("luks2", "Decryption with keyslot 
> %"PRIuGRUB_UINT64_T" failed: %s\n",
>      +                        keyslot_key, grub_errmsg);
>                 continue;
>               }
>       
>     -@@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
>     +       ret = luks2_verify_key (&digest, candidate_key, keyslot.key_size);
>             if (ret)
>               {
>     -           grub_dprintf ("luks2", "Could not open keyslot 
> %"PRIuGRUB_SIZE": %s\n",
>     +-          grub_dprintf ("luks2", "Could not open keyslot 
> %"PRIuGRUB_SIZE": %s\n",
>      -                        i, grub_errmsg);
>     ++          grub_dprintf ("luks2", "Could not open keyslot 
> %"PRIuGRUB_UINT64_T": %s\n",
>      +                        keyslot_key, grub_errmsg);
>                 continue;
>               }
>     @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
>              * where each element is either empty or holds a key.
>              */
>      -      grub_printf_ (N_("Slot %"PRIuGRUB_SIZE" opened\n"), i);
>     -+      grub_printf_ (N_("Slot %"PRIuGRUB_SIZE" opened\n"), keyslot_key);
>     ++      grub_printf_ (N_("Slot %"PRIuGRUB_UINT64_T" opened\n"), 
> keyslot_key);
>       
>             candidate_key_len = keyslot.key_size;
>             break;
> 2:  137909929 = 2:  3baffdd4f luks2: grub_cryptodisk_t->total_length is the 
> max number of device native sectors
> 3:  9cfbb3373 = 3:  6da3d8598 cryptodisk: Fix cipher IV mode 'plain64' always 
> being set as 'plain'.
> 4:  5f7bd00a6 = 4:  fd7cb6b16 cryptodisk: Properly handle non-512 byte sized 
> sectors.
> 5:  4251c828a = 5:  b33733199 cryptodisk: Replace some literals with 
> constants in grub_cryptodisk_endecrypt.
> 6:  feb8298b8 = 6:  de4f6b2e5 cryptodisk: Rename total_length field in 
> grub_cryptodisk_t to total_sectors.
> 7:  e26aed2ee ! 7:  a165791de cryptodisk: Rename offset in grub_cryptodisk_t 
> to offset_sectors.
>     @@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char 
> *check_uu
>      
>       ## grub-core/disk/luks2.c ##
>      @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
>     -       grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", 
> keyslot_key);
>     +       grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_UINT64_T"\n", 
> keyslot_key);
>       
>             /* Set up disk according to keyslot's segment. */
>      -      crypt->offset = grub_divmod64 (segment.offset, 
> segment.sector_size, NULL);
> 8:  e083b25e2 = 8:  86beb5be8 luks2: Rename source disk variabled named 
> 'disk' to 'source' as in luks.c.
> -- 
> 2.27.0
>

signature.asc
Description: PGP signature