From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1npHSm-0002ge-2z for mharc-grub-devel@gnu.org; Thu, 12 May 2022 18:39:16 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:43240) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1npHSi-0002gT-JO for grub-devel@gnu.org; Thu, 12 May 2022 18:39:13 -0400 Received: from mail-vs1-xe2f.google.com ([2607:f8b0:4864:20::e2f]:46842) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1npHSf-0003m3-8y for grub-devel@gnu.org; Thu, 12 May 2022 18:39:11 -0400 Received: by mail-vs1-xe2f.google.com with SMTP id z144so6636908vsz.13 for ; Thu, 12 May 2022 15:39:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=xpupZDA8goUutJ0SgDYfb00tTqJaHEvOqqztd42aIzY=; b=A12sJnS7rvsUYH/oYBDS+L0sDDbkqvWRSp1mChf1Kt+5MBsrneblGmAPjh5ABOMwVn ADDwhkHz/ArQrNE4JPUstGnzWrH62oO+qLONB4OpPTfsQBK7KP8eDVLz8jthaW2uHhCu ybJQUnHXPFyLsBILtPWHB31n94iLQIFWRZTqWfW2zb7UsIf5QUv8HckLwzXzEuNoZWjG agAEOyFAx6YmDfRdPpKqASIF34vXrnLF4KQPKBdxkpZkX/bAXr9v9YZZ9NLhrDTkS64v S2cSYQSuKBHrQjAm7fkzoCYshNsTyIBZEasNXKr3RN1JA57pfJxv594BhFUXyX8uJRv8 90vQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=xpupZDA8goUutJ0SgDYfb00tTqJaHEvOqqztd42aIzY=; b=bgm/Ty7l++bXML/QevUT7l38z67N7vasHiXDBI8oDiE/UO0TBA7ziR6E8yt24IBtmB 0ustlpJxV8tK0S9lA5E6tYU5oonCwkOAdI0J6imwE9Z0ZE1x8Jr5HbJ/qjhYEeyR92U8 aHnfHG0mq5EX39BndgulVjcPYFwRMhEkEOTLr8PNjL0TncJF2zuqykm68uWcbBL4CclL ZbfDJIu9JD0tgAWkME839h2NxW0xVfvCDKbwnkpZC/tc3TkHij4AToOK7VVZ/tH4I4jC NcZ035d8Y0Vw/d097GSFzj9uIRSGW07+mHjEBZPEN/20V4myQ0xAgd9YpZ5E6bo9w5sD IZEA== X-Gm-Message-State: AOAM533YMxrgJlGhKQcCdxR+F85eEl8a7u+c5P/f5+wXy+fEUfWLT9Pj HPapu1baidPbCvp7zVinozs/7NmqEwtUG9aD X-Google-Smtp-Source: ABdhPJxondsZM/0ckgLjmg9IJLgpIthDkW2pHpF7t9jm8aPdK1e9mGQOjaUr+cwKN6VvgGU2zE9A0Q== X-Received: by 2002:a67:d61a:0:b0:32d:4c95:36d5 with SMTP id n26-20020a67d61a000000b0032d4c9536d5mr1331072vsj.36.1652395147949; Thu, 12 May 2022 15:39:07 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([37.218.244.249]) by smtp.gmail.com with ESMTPSA id s8-20020a1f5e08000000b0034e6f1fd050sm81527vkb.26.2022.05.12.15.39.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 May 2022 15:39:07 -0700 (PDT) Date: Thu, 12 May 2022 17:38:52 -0500 From: Glenn Washburn To: Josselin Poiret Cc: grub-devel@gnu.org Subject: Re: [PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters Message-ID: <20220512173852.5cb23d65@crass-HP-ZBook-15-G2> In-Reply-To: <20211211122945.6326-3-dev@jpoiret.xyz> References: <20211209141526.002e83a4@crass-HP-ZBook-15-G2> <20211211122945.6326-1-dev@jpoiret.xyz> <20211211122945.6326-3-dev@jpoiret.xyz> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::e2f; envelope-from=development@efficientek.com; helo=mail-vs1-xe2f.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, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2022 22:39:14 -0000 On Sat, 11 Dec 2021 13:29:45 +0100 Josselin Poiret wrote: > This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes > filled out, otherwise they wouldn't be initialized if cheat mounted. > --- > grub-core/osdep/devmapper/getroot.c | 99 ++++++++++++++++++++++++++++- > 1 file changed, 98 insertions(+), 1 deletion(-) > > diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c > index ad1daf9c8..4b3458639 100644 > --- a/grub-core/osdep/devmapper/getroot.c > +++ b/grub-core/osdep/devmapper/getroot.c > @@ -51,6 +51,8 @@ > #include > #include > > +#include > + > static int > grub_util_open_dm (const char *os_dev, struct dm_tree **tree, > struct dm_tree_node **node) > @@ -183,7 +185,6 @@ grub_util_pull_devmapper (const char *os_dev) > && lastsubdev) > { > char *grdev = grub_util_get_grub_dev (lastsubdev); > - dm_tree_free (tree); > if (grdev) > { > grub_err_t err; > @@ -191,7 +192,103 @@ grub_util_pull_devmapper (const char *os_dev) > if (err) > grub_util_error (_("can't mount encrypted volume `%s': %s"), > lastsubdev, grub_errmsg); > + if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0) > + { > + /* set LUKS2 cipher and size from dm parameter, since it is not > + * possible to determine the correct ones without unlocking, as > + * there might be multiple segments. > + */ > + grub_disk_t source = grub_disk_open (grdev); > + grub_cryptodisk_t cryptodisk = grub_cryptodisk_get_by_source_disk (source); > + grub_disk_close (source); > + grub_addr_t start, length; > + char *target_type = NULL; > + char *params; > + const char *name = dm_tree_node_get_name (node); > + char *cipher, *cipher_mode; > + struct dm_task *dmt; I think these declarations should be before non-declaration statements, eg. grub_disk_close(). On the other hand, Daniel has been known to want declarations at the start of the function block. > + grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'", > + uuid, name); > + if (!(dmt = dm_task_create (DM_DEVICE_TABLE))) This assignment should be a separate statement to more conform to the coding style of GRUB. > + grub_util_error (_("can't create dm task DM_DEVICE_TABLE")); > + if (!dm_task_set_name (dmt, name)) This condition should be dm_task_set_name (dmt, name) == 0. And same for similar usages below. > + grub_util_error (_("can't set dm task name to `%s'"), name); > + if (!dm_task_run (dmt)) > + grub_util_error (_("can't run dm task for `%s'"), name); > + dm_get_next_target (dmt, NULL, &start, &length, > + &target_type, ¶ms); It looks like this might return NULL on error? Or does it never error here? > + if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0) > + grub_util_error (_("dm target is not `crypt'")); It would be nice for debugging to know what type the dm target was on error. So I'd rather this line: grub_util_error (_("dm target of type `%s' is not type `crypt'"), target_type); > + > + cryptodisk->total_sectors = length; I believe this is in device mapper sector sized sectors, which is fine, until we find out below that the volume is using a non-default sector size. > + cryptodisk->log_sector_size = 9; /* default dm sector size */ The comment is nice, but probably better is to create a macro called DM_LOG_SECTOR_SIZE. > + > + /* dm target parameters for dm-crypt is > + * [<#opt_params> ] > + */ > + char *seek_head, *c; > + c = params; > + > + /* first, get the cipher name from the cipher */ > + if (!(seek_head = grub_memchr (c, '-', grub_strlen (c)))) I would have have grub_strlen() called once on this string and that value decremented appropriately for the rest below. > + grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"), > + params); > + cipher = grub_strndup (c, seek_head - c); > + c = seek_head + 1; > + > + /* now, the cipher mode */ > + if (!(seek_head = grub_memchr (c, ' ', grub_strlen (c)))) > + grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"), > + params); > + cipher_mode = grub_strndup (c, seek_head - c); > + > + grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode); > + > + /* This is the only hash usable by PBKDF2, so set it as such */ > + cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256"); So does this mean that when/if argon2 support is added, that this will need to be detected as well? If so, is there a way to get the hash via devmapper? It doesn't show up with the table command, so I'm guessing not. On the otherhand, I don't think cryptodisk->hash is ever used in the user-space utilities because the cryptodisk is never attempted to be unlocked, so there's no password to hash. Perhaps this should be NULL with a comment that its not used (if I'm right about that). > + > + /* drop key, iv_offset, device path and offset */ > + for (int dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++) > + { > + seek_head++; > + seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head)); Instead of these two lines, I believe it could be: seek_head = grub_memchr (seek_head + 1, ' ', grub_strlen (seek_head)); > + } > + > + /* if we have optional parameters, skip #opt_params */ > + if (seek_head && (seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head)))) If seek_head != NULL, then it points to space because of the grub_memchr() in the for loop above. So calling grub_memchr() again is pointless, right? ie. seek_head will never change in the assignment above. I think this if above can be removed and just use the for loop below, and in the initializer section have seek_head++. > + { > + seek_head++; > + for (;seek_head;seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head))) > + { > + seek_head++; On the first iteration in the loop seek_head starts pointing at an optional parameter, then the line above will increment the pointer to point to the second byte of the optional parameter. If the optional parameter is the one we're looking for, eg. sector_size, we will not find a match. This code only works because the sector_size optinoal parameter is rarely first, but this should not be assumed. The seek_head++ should happen at the end or in the loop update part of the for loop above. > + if (strncmp (seek_head, "sector_size:", sizeof ("sector_size:") - 1) == 0) > + { > + char *sector_size_str; > + unsigned long long sector_size; > + c = seek_head + sizeof ("sector_size:") - 1; > + seek_head = grub_memchr (c, ' ', grub_strlen (c)); > + if (seek_head) > + sector_size_str = grub_strndup (c, seek_head - c); This has no corresponding grub_free(). > + else > + sector_size_str = c; > + sector_size = grub_strtoull (sector_size_str, NULL, 10); This should check for in grub_strtoull errors as is done in commit ac8a37dda (net/http: Allow use of non-standard TCP/IP ports). > + > + if (sector_size != 512 && sector_size != 1024 && > + sector_size != 2048 && sector_size != 4096) > + grub_util_error (_("dm-crypt parameter sector_size `%s' is not a valid LUKS2 sector size"), > + sector_size_str); > + cryptodisk->log_sector_size = grub_log2ull (sector_size); Now that the log_sector_size is changed from the default above, length should also be updated to reflect the sector count in the newly detected sector size. > + grub_util_info ("set sector_size for dm `%s' to `%d'", s/dm/cryptodisk/ > + name, 1 << cryptodisk->log_sector_size); > + break; > + } > + } > + } > + > + dm_task_destroy (dmt); > + } > } > + dm_tree_free (tree); > grub_free (grdev); > } > else Glenn