[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/7] Cryptomount support LUKS detached header
From: |
TJ |
Subject: |
Re: [PATCH 1/7] Cryptomount support LUKS detached header |
Date: |
Thu, 22 Mar 2018 14:22:13 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 22/03/18 12:38, Daniel Kiper wrote:
> Hi John,
>
> On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote:
>> On 14/03/18 13:05, Daniel Kiper wrote:
>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote:
>>>> From: John Lane <address@hidden>
>>>
>>> I have just skimmed through the series. First of all, most if not
>>> all patches beg for full blown commit messages. Just vague statements
>>> in the subject are insufficient for me. And please add patch 0 which
>>> introduces the series. git send-email --compose is your friend.
>>>
>>> Daniel
>>>
>>
>> Sorry Daniel, this isn't something I do that often - submitting patches
>
> Not a problem.
>
>> to ML. Do you want me to resubmit them again, or is the below ok for you ?
>
> Please resubmit whole patch series and do not forget to take into
> account comments posted by others.
>
> Daniel
I've spent a couple of days doing a thorough review of this patch series.
Firstly I want to say a big thanks to John for bringing this along -
it's long been a large missing piece of the LUKS support.
My observations:
Break the series up. There are five distinct sets of functionality
change here:
a) LUKS detached headers
b) LUKS key files
c) allow multiple unlock attempts
d) plain dm-crypt
e) hyphens in UUIDs
(a) and (b) are in reasonable shape but there's some work to do; mostly
in preparing the way for the new functionality by cleaning up error exit
paths in luks.c::luks_recover_key() first - which I've done and will attach.
With that clean-up John's changes are easier to insert and verify.
(c) creates a lot of churn just due to indenting code that is being
wrapped in a while() loop. I've refactored that so the loop is in a
separate function which reduces the patch from 323 to 41 lines. It's in
my branch detailed below.
I'd suggest submitting (a) (b) and (c) as a series on their own. Get
them accepted and then introduce (e) and (d). I'd say (e) first since
it's relatively small.
(d) is a major new tranch of functionality dealing with core
cryptographic constructs so will need careful review by cryptographers
as well as GRUB developers and therefore could take some time. It'd be a
shame to have this hold up the excellent improvements to LUKS which
don't touch the cryptographic operations.
All in all an excellent contribution which I look forward to being
available.
My "cryptomount_luks_v5" branch for the LUKS changes can be got using:
git remote add iam.tj git://iam.tj/grub.git
git fetch iam.tj
and seen at:
http://iam.tj/gitweb/?p=grub.git;a=shortlog;h=refs/heads/cryptomount_luks_v5
---
commit 19896820737344fb820ab6487d16719e31cae763
Author: TJ <address@hidden>
Date: Wed Mar 21 14:07:22 2018 +0000
LUKS: refactor multiple return paths
Convert multiple return statements to goto with a single return so there
is only one place where memory needs to be free-d in error conditions.
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 86c50c6..a7c5b39 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -318,18 +318,23 @@ luks_recover_key (grub_disk_t source,
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
unsigned i;
grub_size_t length;
- grub_err_t err;
+ grub_err_t err = GRUB_ERR_NONE;
+ char *err_msg = NULL;
grub_size_t max_stripes = 1;
char *tmp;
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
- return err;
+ goto fail;
grub_puts_ (N_("Attempting to decrypt master key..."));
keysize = grub_be_to_cpu32 (header.keyBytes);
if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN)
- return grub_error (GRUB_ERR_BAD_FS, "key is too long");
+ {
+ err = GRUB_ERR_BAD_FS;
+ err_msg = "key is too long";
+ goto fail;
+ }
for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED
@@ -338,7 +343,10 @@ luks_recover_key (grub_disk_t source,
split_key = grub_malloc (keysize * max_stripes);
if (!split_key)
- return grub_errno;
+ {
+ err = grub_errno;
+ goto fail;
+ }
/* Get the passphrase from the user. */
tmp = NULL;
@@ -350,8 +358,9 @@ luks_recover_key (grub_disk_t source,
grub_free (tmp);
if (!grub_password_get (passphrase, MAX_PASSPHRASE))
{
- grub_free (split_key);
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+ err = GRUB_ERR_BAD_ARGUMENT;
+ err_msg = "Passphrase not supplied";
+ goto fail;
}
/* Try to recover master key from each active keyslot. */
@@ -378,8 +387,8 @@ luks_recover_key (grub_disk_t source,
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
grub_dprintf ("luks", "PBKDF2 done\n");
@@ -387,8 +396,8 @@ luks_recover_key (grub_disk_t source,
gcry_err = grub_cryptodisk_setkey (dev, digest, keysize);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
@@ -399,16 +408,13 @@ luks_recover_key (grub_disk_t source,
[i].keyMaterialOffset), 0,
length, split_key);
if (err)
- {
- grub_free (split_key);
- return err;
- }
+ goto fail;
gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
/* Merge the decrypted key material to get the candidate master
key. */
@@ -416,8 +422,8 @@ luks_recover_key (grub_disk_t source,
grub_be_to_cpu32 (header.keyblock[i].stripes));
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
grub_dprintf ("luks", "candidate key recovered\n");
@@ -433,8 +439,8 @@ luks_recover_key (grub_disk_t source,
sizeof (candidate_digest));
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
/* Compare the calculated PBKDF2 to the digest stored
@@ -454,17 +460,19 @@ luks_recover_key (grub_disk_t source,
gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize);
if (gcry_err)
{
- grub_free (split_key);
- return grub_crypto_gcry_error (gcry_err);
+ err = grub_crypto_gcry_error (gcry_err);
+ goto fail;
}
- grub_free (split_key);
-
- return GRUB_ERR_NONE;
+ err = GRUB_ERR_NONE;
}
+fail:
grub_free (split_key);
- return GRUB_ACCESS_DENIED;
+ if(err && err_msg)
+ grub_error (err, errmsg);
+
+ return err;
}
struct grub_cryptodisk_dev luks_crypto = {
- Re: [PATCH 2/7] Cryptomount support key files, (continued)
Re: [PATCH 1/7] Cryptomount support LUKS detached header, TJ, 2018/03/17