From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1nsCrQ-0001X4-Am for mharc-grub-devel@gnu.org; Fri, 20 May 2022 20:20:48 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33172) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nsCrO-0001T8-1Q for grub-devel@gnu.org; Fri, 20 May 2022 20:20:46 -0400 Received: from mail-ua1-x92f.google.com ([2607:f8b0:4864:20::92f]:41542) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1nsCrL-0006UJ-O3 for grub-devel@gnu.org; Fri, 20 May 2022 20:20:45 -0400 Received: by mail-ua1-x92f.google.com with SMTP id 90so3483375uam.8 for ; Fri, 20 May 2022 17:20:43 -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=nzgJKeJwg260Gz8CSlQALQd4ZepCUy63XPy271E9Jh4=; b=2wSiEDffx0jQ2L4JxrM+nBEkPeQmd4OsWJgBCP78+S4Hb9pr7aeo/o6a8n6bgGFoWy WzfpktnTPhSxpftDRkjerVmcOPBal0DPoLkN2G7NXZr/iODNtJCyHnhparYHsei3Pqqd 5iOWvCORDQSB/ZzK/DdSQ45Z2ssdyvMMokEbS6rRTSNYIF4QVprz5+yU9HOJL/SstnKJ iVvxxSgbuPQcI4ID8TrQz/PIGgYnLMe0VPG3ya7rJsnffPS0C1JZ4Jsf3EyB+k+2gurP 4rcBl1MrybGa84dpjZv92SeprJXRTEnil0NpqG3FJDRCJp4AAEXAc7IuVWYYD1IVeesS 3iJA== 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=nzgJKeJwg260Gz8CSlQALQd4ZepCUy63XPy271E9Jh4=; b=autIaxbqXungcwc+h4tj0G4BdmX+ZxQiyt/u1miiCVgaT8Z8/rCv+43LzXtZzR0M/Z QbJ+e0EjNfpFgZ+V00oJeGqPtIGvlf3RCpa9k2dSRA4OXXWYcXtcSr19V81WHL0uSwbU tKxXP95f+GrZDfm+ik2y+Bo/zTNyKQgjDCfjUPm5FahP1i5yR/lS4HzNUBUdsPnP7E0g RIJJbN7UduxyOvB0xm/b2pYAxX8ethNcvHhcHPiDyymu+NKdvuy/lyDVRUaLQ1pzHseM 6sarcE+nR6i9BDWx2XnoqTwdmj44sNHa5s51cYjxuiz1vGcxoBxAN5UNamlXqj/8ZUxz tDqg== X-Gm-Message-State: AOAM532n9QQ98bRTm0iBLQt+aYKxggCtlaN7GuBI94oSoZQ1MI26tIL+ oegnfGHJDEI/nJy7/5s28/mcO5LLEM2w6A== X-Google-Smtp-Source: ABdhPJxEi3EUfxM0OdqYdogn7TQxAI03vJEKKnjDBkLX9qfEzMWid1W9NUEZK9ZsRW8Ri2/7pTpWXA== X-Received: by 2002:ab0:7501:0:b0:368:97ec:fafa with SMTP id m1-20020ab07501000000b0036897ecfafamr4591383uap.5.1653092442303; Fri, 20 May 2022 17:20:42 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([37.218.244.249]) by smtp.gmail.com with ESMTPSA id w14-20020a9f31ce000000b00368cee215b4sm96271uad.18.2022.05.20.17.20.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 May 2022 17:20:42 -0700 (PDT) Date: Fri, 20 May 2022 19:20:35 -0500 From: Glenn Washburn To: Josselin Poiret Cc: grub-devel@gnu.org Subject: Re: [PATCH v3 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters Message-ID: <20220520192035.313b5ede@crass-HP-ZBook-15-G2> In-Reply-To: <20220520182039.21654-3-dev@jpoiret.xyz> References: <20220512173852.5cb23d65@crass-HP-ZBook-15-G2> <20220520182039.21654-1-dev@jpoiret.xyz> <20220520182039.21654-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::92f; envelope-from=development@efficientek.com; helo=mail-ua1-x92f.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: Sat, 21 May 2022 00:20:46 -0000 Thanks for making these updates. On Fri, 20 May 2022 20:20:39 +0200 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 | 130 +++++++++++++++++++++++++++- > 1 file changed, 129 insertions(+), 1 deletion(-) > > diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c > index ad1daf9c8..b8d66029a 100644 > --- a/grub-core/osdep/devmapper/getroot.c > +++ b/grub-core/osdep/devmapper/getroot.c > @@ -43,6 +43,8 @@ > #include > #include > > +#define DM_LOG_SECTOR_SIZE 9 > + > #include > #include > > @@ -51,6 +53,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 +187,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 +194,132 @@ 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 parameters, since it is not > + * possible to determine the correct ones without unlocking, as > + * there might be multiple segments. > + */ > + grub_disk_t source; > + grub_cryptodisk_t cryptodisk; > + grub_addr_t start, length; > + char *target_type; > + char *params; > + const char *name; > + char *cipher, *cipher_mode; > + struct dm_task *dmt; > + > + source = grub_disk_open (grdev); > + cryptodisk = grub_cryptodisk_get_by_source_disk (source); > + grub_disk_close (source); > + > + name = dm_tree_node_get_name (node); > + > + grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'", > + uuid, name); > + > + dmt = dm_task_create (DM_DEVICE_TABLE); > + if (dmt == 0) > + grub_util_error (_("can't create dm task DM_DEVICE_TABLE")); > + if (dm_task_set_name (dmt, name) == 0) > + grub_util_error (_("can't set dm task name to `%s'"), name); > + if (dm_task_run (dmt) == 0) > + grub_util_error (_("can't run dm task for `%s'"), name); > + /* dm_get_next_target doesn't have any error modes, everything has > + * been handled by dm_task_run. > + */ > + dm_get_next_target (dmt, NULL, &start, &length, > + &target_type, ¶ms); > + if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0) > + grub_util_error (_("dm target of type `%s' is not `crypt'"), > + target_type); > + > + cryptodisk->total_sectors = length; > + cryptodisk->log_sector_size = DM_LOG_SECTOR_SIZE; > + > + /* dm target parameters for dm-crypt is > + * [<#opt_params> ...] > + */ > + char *seek_head, *c; > + unsigned int remaining; Daniel might want these declared up with the rest at the start of the block. > + c = params; > + remaining = grub_strlen (c); > + > + /* first, get the cipher name from the cipher */ > + if (!(seek_head = grub_memchr (c, '-', remaining))) > + grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"), > + params); > + cipher = grub_strndup (c, seek_head - c); > + remaining -= seek_head - c; I think remaining should technically be one less here because of the line below so that it can be used in the following grub_memchr() as is. > + c = seek_head + 1; > + > + /* now, the cipher mode */ > + if (!(seek_head = grub_memchr (c, ' ', remaining))) > + grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"), > + params); > + cipher_mode = grub_strndup (c, seek_head - c); > + remaining -= seek_head - c; Ditto. Now we're off by 2 after the next line. > + c = seek_head + 1; > + > + grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode); Probably should error check the return here. > + > + grub_free (cipher); > + grub_free (cipher_mode); > + > + /* This is the only hash usable by PBKDF2, so set it as such */ > + cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256"); This should do an error check for if (cryptodisk->hash == NULL). My prior review had a comment here on if the hash might change if/when argon2 is supported. I'm thinking that we can visit that when argon2 support gets added. So I'm fine with having hash be a constant. > + > + /* drop key, iv_offset, device path and offset */ > + for (char dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++) Why did dropped_tokens get changed to a "char"? Does it really buy us anything? > + { Probably should check for (seek_head == NULL), just in case. Perhaps if true do: grub_util_error (_("dm-crypt parameters missing required fields `%s'"), params); I'm suggesting this above the grub_memchr() so that we don't check the last iteration of the loop, in which seek_head == NULL is not an error. If you want to restructure this some other way, go ahead. > + seek_head = grub_memchr (c, ' ', remaining); > + remaining -= seek_head - c; Ditto, on an off-by-one error here. These will accumulate such that grub_memchr() may return a position outside the bounds of the params string. > + c = seek_head + 1; > + } > + > + /* if we have optional parameters, skip #opt_params */ This comment doesn't quite make sense to me. What is #opt_params? I presume "number of optional parameters", but the code doesn't skip optional parameters. It searches through the optional parameters for sector_size, and once it finds it, then breaks out. It seems like it should be something like: /* if we have optional parameters, search for sector_size */ This comment would still be good even if getting rid of the if block enclosing the while loop as suggested below. > + if (seek_head && (seek_head = grub_memchr (c, ' ', remaining))) > + { > + remaining -= seek_head - c; > + c = seek_head + 1; I still don't see the utility of these lines above, along with the "if" statement. You should be able to remove them, thus having the while outside of this block, and still the code would be correct. The check for seek_head != NULL can be done in the while condition. If there are optional parameters, then seek_head should be non-NULL and "c" should point to the first character of the first listed optional parameter, assuming all parameters are separated by a single space. Even if that is not true and there are multiple spaces between some parameters, the while loop will handle it fine, just perhaps not as efficiently as it could. > + while (remaining > 0) > + { > + if (strncmp (c, "sector_size:", sizeof ("sector_size:") - 1) == 0) If we're concerned about multiple spaces between parameters, we could make this more efficient by using this conditional instead: if (*c != ' ' && strncmp (c, "sector_size:", sizeof ("sector_size:") - 1) == 0) This way the strncmp only runs if the character c points to is not a space. I'm not really concerned about extra spaces though and am fine with the inefficiency if there are. > + { > + char *sector_size_str; > + unsigned long long sector_size; > + c += sizeof ("sector_size:") - 1; > + remaining -= sizeof ("sector_size:") - 1; > + seek_head = grub_memchr (c, ' ', remaining); > + if (seek_head) > + sector_size_str = grub_strndup (c, seek_head - c); > + else > + sector_size_str = grub_strndup (c, remaining); > + sector_size = grub_strtoull (sector_size_str, NULL, 10); This > + grub_free (sector_size_str); > + > + 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); > + cryptodisk->total_sectors = cryptodisk->total_sectors / > + (sector_size / (1 << DM_LOG_SECTOR_SIZE)); Should use grub_convert_sector() in include/grub/disk.h, eg: cryptodisk->total_sectors = grub_convert_sector (cryptodisk->total_sectors, cryptodisk->log_sector_size, DM_LOG_SECTOR_SIZE); > + grub_util_info ("set sector_size for cryptodisk `%s' to `%d', total_sectors to `%lu'", > + name, 1 << cryptodisk->log_sector_size, could use sector_size instead of 1 << cryptodisk->log_sector_size > + cryptodisk->total_sectors); > + break; > + } > + seek_head = grub_memchr (c, ' ', remaining); > + remaining -= seek_head - c; If none of the optional parameters start with "sector_size:" and remaining is correct, then we'll come to the last optional parameter and grub_memchr() will return NULL. Then remaining will be (remaining - ( 0 - c )) or (remaining + c), where c is likely a large number. Then the while condition, remaining > 0, will continue to be true and go through another iteration. But onthis next iteration c will be pointing to address 1 because of the line below. Accessing *c at that point might cause a crash or later on as the train has been derailed. > + c = seek_head + 1; > + } > + } > + > + dm_task_destroy (dmt); > + } > } > + dm_tree_free (tree); > grub_free (grdev); > } > else After reviewing this, I stumbled upon Fabian's patch. I wrote a response on the thread of that patch with the suggestion that his patch be used to get the total number of sectors and sector size and this patch be used to get the crypto information. This would obviate the problematic parsing part of this patch. What do you think about removing all the parsing code after the setting of cryptodisk->hash? Glenn