From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kh4Kb-0008CK-Ob for mharc-grub-devel@gnu.org; Mon, 23 Nov 2020 00:24:05 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:40532) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kh4Ka-0008C4-BY for grub-devel@gnu.org; Mon, 23 Nov 2020 00:24:04 -0500 Received: from mail-wm1-x32d.google.com ([2a00:1450:4864:20::32d]:52498) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kh4KX-0006RJ-Kv for grub-devel@gnu.org; Mon, 23 Nov 2020 00:24:04 -0500 Received: by mail-wm1-x32d.google.com with SMTP id 10so16732006wml.2 for ; Sun, 22 Nov 2020 21:24:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=Hz/La18XUxr8wt4MXAaG4nA3Y1Ukz27rbFzxSkXG6Ls=; b=JvS+2sozC0HDOZI7DY+qIAVeUkOpl5uE5mI+0ngFRM50btJXpei2KbK1jypGh38PBb eCUQbm3klL320Scz39sjSB7IayDOO+Sy3ZHNP5qWS4GGXGpJZIgDY0ohr2vx7Iekn2CF 8Bom9s/Hb1yo/O5cf8vlkQcIC1wTW06Z89gQ7ODutWuRq+Ak3YBnOfbjwJkCS1d173Zb lS1dxI+0k8QPW074mX1IKsmos30K/xwvcQdXwx3Mi8Vfmt6aVfwyjZqx8DA6CKHg2BYR 1pZLNGHnSjKZ5esP9mnPmz3YGrGfdqd2wJywyq5O8sRrSZav/Epu7hkOVxFeaJF0s6H3 vG/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=Hz/La18XUxr8wt4MXAaG4nA3Y1Ukz27rbFzxSkXG6Ls=; b=ILx+vzAbWHUmTD8Tm8QI1JuLu5q7+YCNEMYIKPlAn6clk7FR4hC47KFm+c4SaBT9ps /aw8ICdcZRI9CyhnTku8A+7/Zwf/XqHDIaMHN/nzu/IO+QtlpEraNM8mG64G+T6o3nG/ BLNi1E7F3H/W46gRbUjEQAJ5HGmx6EnbukmMKPM91rS8L5r4cFfJV6CyfOTiVdi12zQK 9AJfIfh7Gh0VLAtX6OovYnBasq1d7g4uyA2O1EmRFn84Q7xXtI5kYLNGX7kldOkfAceS Ppt16QDZPg8tMhunbgQbI8lkpwL35RlIpJz2HWJ007BnCgP9rWkpt5MKjgnfmyQWSZNM w42A== X-Gm-Message-State: AOAM531/T4uN62umfn5+WBcedS/H0KOzqW3cM4hfZXzXQa8zO+iwRNt+ lSJ/6OULydiZX8Kj2Im0T7O61BceHE3tUQ== X-Google-Smtp-Source: ABdhPJwwnaWtv/zx2oucdzm3XSkORgSJPf3ATTIPOT6y9t/EWEy/KFnWEXPEoGEIiBhSZDnPpmLx+A== X-Received: by 2002:a1c:7412:: with SMTP id p18mr21643529wmc.8.1606109038898; Sun, 22 Nov 2020 21:23:58 -0800 (PST) Received: from localhost.localdomain ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id f16sm11631780wmh.7.2020.11.22.21.23.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 22 Nov 2020 21:23:58 -0800 (PST) From: Glenn Washburn To: grub-devel@gnu.org Cc: Patrick Steinhardt , Daniel Kiper , Glenn Washburn Subject: [PATCH v5 00/11] Cryptodisk fixes for v2.06 redux Date: Sun, 22 Nov 2020 23:23:13 -0600 Message-Id: X-Mailer: git-send-email 2.27.0 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Received-SPF: pass client-ip=2a00:1450:4864:20::32d; envelope-from=development@efficientek.com; helo=mail-wm1-x32d.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2020 05:24:04 -0000 This incorporates changes as noted in the v4 thread. This series has been rebased on the new master from accepted patches from v4. This means that patch 8 from v4 would be patch 1 in this series. However, I reworked 8-10 from v4 from 3 patches into 2, and I believe simplified the changeset. So v4 patches 8-10 are equivalent to patches 1-2 here. Also, the v4 patch 14 has been nearly completely reworked to be easier to follow and more precisely reflect the error handling I wanted. The changes that created and used grub_log2ull in v4 patch 14 has been pulled out into two separate patches. And there is a new patch converting spaces to tabs missed in the original commit of luks2. Glenn Glenn Washburn (11): luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest luks: Use more intuitive slot key instead of index in user messages. cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt luks2: grub_cryptodisk_t->total_sectors is the max number of device native sectors cryptodisk: Properly handle non-512 byte sized sectors luks2: Better error handling when setting up the cryptodisk luks2: Error check segment.sector_size whitespace: convert 8 spaces to tabs. mips: Enable __clzdi2() misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers luks2: Use grub_log2ull to calculate log_sector_size and improve readability grub-core/disk/cryptodisk.c | 64 ++++++++++------ grub-core/disk/luks.c | 5 +- grub-core/disk/luks2.c | 144 ++++++++++++++++++++++++++++------- grub-core/kern/compiler-rt.c | 2 +- include/grub/compiler-rt.h | 2 +- include/grub/cryptodisk.h | 8 +- include/grub/disk.h | 16 ++++ include/grub/misc.h | 3 + include/grub/types.h | 5 ++ 9 files changed, 192 insertions(+), 57 deletions(-) Range-diff against v4: 1: f1d530757 < -: --------- luks2: Split idx into three variables: keyslot_key, digest_key, segment_key. 2: b90ab8e75 < -: --------- luks2: Improve error messages in luks2_get_keyslot. -: --------- > 1: ec506b668 luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest 3: 51add6875 ! 2: 3a9fca36f luks2: Use more intuitive keyslot key instead of index when naming keyslot. @@ Metadata Author: Glenn Washburn ## Commit message ## - luks2: Use more intuitive keyslot key instead of index when naming keyslot. + luks: Use more intuitive slot key instead of index in user messages. - Use the keyslot key value in the keyslot json array rather than the index of - the keyslot in the json array. This is less confusing for the end user. For - example, say you have a LUKS2 device with a key in slot 1 and slot 4. When - using the password for slot 4 to unlock the device, the messages using the - index of the keyslot will mention keyslot 1 (its a zero-based index). - Furthermore, with this change the keyslot number will align with the number - used to reference the keyslot when using the --key-slot argument to - cryptsetup. + Use the slot key name in the json array rather than the 0 based index in the + json array for keyslots, segments, and digests. This is less confusing for + the end user. For example, say you have a LUKS2 device with a key in slot 1 + and slot 4. When using the password for slot 4 to unlock the device, the + messages using the index of the keyslot will mention keyslot 1 (its a + zero-based index). Furthermore, with this change the keyslot number will + align with the number used to reference the keyslot when using the + --key-slot argument to cryptsetup. Error messages now distinguish between + indexes and slot keys. The former include the string "index" in the error + string, and the later are surrounded in quotes. ## grub-core/disk/luks2.c ## -@@ grub-core/disk/luks2.c: typedef struct grub_luks2_header grub_luks2_header_t; - - struct grub_luks2_keyslot - { -+ grub_uint64_t slot_key; - grub_int64_t key_size; - grub_int64_t priority; - struct -@@ grub-core/disk/luks2.c: typedef struct grub_luks2_keyslot grub_luks2_keyslot_t; - - struct grub_luks2_segment - { -+ grub_uint64_t slot_key; - grub_uint64_t offset; - const char *size; - const char *encryption; -@@ grub-core/disk/luks2.c: typedef struct grub_luks2_segment grub_luks2_segment_t; - - struct grub_luks2_digest - { -+ grub_uint64_t slot_key; - /* Both keyslots and segments are interpreted as bitfields here */ - grub_uint64_t keyslots; - grub_uint64_t segments; @@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s - { - grub_json_t keyslots, keyslot, digests, digest, segments, segment; - grub_size_t i, size; -- grub_uint64_t keyslot_key, digest_key, segment_key; -+ grub_uint64_t digest_key, segment_key; - - /* Get nth keyslot */ - if (grub_json_getvalue (&keyslots, root, "keyslots") || - grub_json_getchild (&keyslot, &keyslots, keyslot_idx) || -- grub_json_getuint64 (&keyslot_key, &keyslot, NULL) || -+ grub_json_getuint64 (&k->slot_key, &keyslot, NULL) || + grub_json_getuint64 (&k->slot_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 index %"PRIuGRUB_SIZE, keyslot_idx); +- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot %"PRIuGRUB_SIZE, keyslot_idx); ++ 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-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s + grub_json_getuint64 (&d->slot_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 index %"PRIuGRUB_SIZE, i); +- 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 index %"PRIuGRUB_SIZE, i); -- if ((d->keyslots & (1 << keyslot_key))) -+ d->slot_key = digest_key; -+ if ((d->keyslots & (1 << k->slot_key))) + if ((d->keyslots & (1 << k->slot_key))) break; } if (i == size) -- return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot \"%"PRIuGRUB_UINT64_T"\"", keyslot_key); +- return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot %"PRIuGRUB_SIZE, keyslot_idx); + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key); /* Get segment that matches the digest. */ if (grub_json_getvalue (&segments, root, "segments") || @@ grub-core/disk/luks2.c: luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s + grub_json_getuint64 (&s->slot_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 index %"PRIuGRUB_SIZE, i); +- 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 index %"PRIuGRUB_SIZE, i); -+ s->slot_key = segment_key; - if ((d->segments & (1 << segment_key))) + if ((d->segments & (1 << s->slot_key))) break; } + if (i == 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_UINT64_T"\"", d->slot_key); + + return GRUB_ERR_NONE; + } @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source, if (keyslot.priority == 0) 4: 39e382053 ! 3: f45f5f3dd cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt. @@ Metadata Author: Glenn Washburn ## Commit message ## - cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt. + cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt This should improve readability of code by providing clues as to what the value represents. The new macro GRUB_TYPE_BITS(type) returns the number of @@ include/grub/types.h: typedef grub_int32_t grub_ssize_t; #endif # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1) -+#define GRUB_TYPE_U_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BITS (type) - 1)) - 1)) + 1) ++#define GRUB_TYPE_U_MAX(type) ((typeof (1ULL))((typeof (type))(~0))) +#define GRUB_TYPE_U_MIN(type) 0ULL + typedef grub_uint64_t grub_properly_aligned_t; 5: 7696a000f ! 4: da2b63607 luks2: grub_cryptodisk_t->total_length is the max number of device native sectors @@ Metadata Author: Glenn Washburn ## Commit message ## - luks2: grub_cryptodisk_t->total_length is the max number of device native sectors + luks2: grub_cryptodisk_t->total_sectors is the max number of device native sectors - The total_length field is named confusingly because length usually refers to - bytes, whereas in this case its really the total number of sectors on the - device. Also counter-intuitively, grub_disk_get_size returns the total - number of device native sectors. We need to convert the sectors from the - size of the underlying device to the cryptodisk sector size. And - segment.size is in bytes which need to be converted to cryptodisk sectors. + We need to convert the sectors from the size of the underlying device to the + cryptodisk sector size; segment.size is in bytes which need to be converted + to cryptodisk sectors as well. And counter-intuitively, grub_disk_get_size + returns the total number of device native sectors. Also, removed an empty statement. 6: 43437eb13 ! 5: 036304262 cryptodisk: Properly handle non-512 byte sized sectors. @@ Metadata Author: Glenn Washburn ## Commit message ## - cryptodisk: Properly handle non-512 byte sized sectors. + cryptodisk: Properly handle non-512 byte sized sectors By default, dm-crypt internally uses an IV that corresponds to 512-byte sectors, even when a larger sector size is specified. What this means is @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk * { grub_size_t sz = ((dev->cipher->cipher->blocksize + sizeof (grub_uint32_t) - 1) - / sizeof (grub_uint32_t)); -- grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4]; -+ grub_uint32_t iv[(GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE + 3) / 4] __attribute__((aligned (sizeof (grub_uint64_t)))); - - if (dev->rekey) - { @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev, if (!ctx) return GPG_ERR_OUT_OF_MEMORY; @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_endecrypt (struct grub_cryptodisk * + * 32 bits. + */ + { -+ grub_uint64_t *iv64 = (grub_uint64_t *) iv; -+ *iv64 = grub_cpu_to_le64 (sector << (log_sector_size ++ grub_uint64_t iv64; ++ ++ iv64 = grub_cpu_to_le64 (sector << (log_sector_size + - GRUB_CRYPTODISK_IV_LOG_SIZE)); ++ grub_set_unaligned64 (iv, iv64); + if (dev->mode_iv == GRUB_CRYPTODISK_MODE_IV_PLAIN) + iv[1] = 0; + } 7: 71dadcd07 < -: --------- luks2: Better error handling when setting up the cryptodisk. -: --------- > 6: 7bf56c4b3 luks2: Better error handling when setting up the cryptodisk 8: cc96baf5a ! 7: b167b014d luks2: Error check segment.sector_size. @@ Metadata Author: Glenn Washburn ## Commit message ## - luks2: Error check segment.sector_size. + luks2: Error check segment.sector_size ## grub-core/disk/luks2.c ## @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source, @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source, + /* Sector size should be one of 512, 1024, 2048, or 4096. */ + if (!(segment.sector_size == 512 || segment.sector_size == 1024 || -+ segment.sector_size == 2048 || segment.sector_size == 4096)) ++ segment.sector_size == 2048 || segment.sector_size == 4096)) + { + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" sector" + " size %"PRIuGRUB_UINT64_T" is not one of" -+ " 512, 1024, 2048, or 4096.", ++ " 512, 1024, 2048, or 4096\n", + segment.slot_key, segment.sector_size); + continue; + } + /* Set up disk according to keyslot's segment. */ crypt->offset_sectors = grub_divmod64 (segment.offset, segment.sector_size, NULL); - crypt->log_sector_size = grub_log2ull (segment.sector_size); + crypt->log_sector_size = sizeof (unsigned int) * 8 -: --------- > 8: c503ecbb8 whitespace: convert 8 spaces to tabs. -: --------- > 9: 8316e94ce mips: Enable __clzdi2() -: --------- > 10: 37257725c misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers -: --------- > 11: 8dd088cad luks2: Use grub_log2ull to calculate log_sector_size and improve readability -- 2.27.0