From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1oMCl9-0004rV-FG for mharc-grub-devel@gnu.org; Thu, 11 Aug 2022 14:18:19 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54634) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oMCl7-0004qj-Qb for grub-devel@gnu.org; Thu, 11 Aug 2022 14:18:18 -0400 Received: from mail-qv1-xf33.google.com ([2607:f8b0:4864:20::f33]:39572) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oMCl5-00020c-2T for grub-devel@gnu.org; Thu, 11 Aug 2022 14:18:17 -0400 Received: by mail-qv1-xf33.google.com with SMTP id h8so13923104qvs.6 for ; Thu, 11 Aug 2022 11:18:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:subject:cc:to:from:date:from:to:cc; bh=ZWXbwMr5kW7JWgQCKS/f5gIk/1m9Kn3wGidgFrizJ6A=; b=otsqT8SiarKQ+iSVUluuu2z67c/Zoos59+CblX2n/nyhoXC5epguSKbMd8hq0lctUT 4Q3+m7PPnNsx9Io/AelhdrcK3FF096IHk5nFK8NStPsrJQYqHmOfjvmIvVN50L8xVf80 1FwpeU5AgQLhNRCC75lMpc143rU8fBe8EXtQCNmIIyfHjI+034TvuxOIeoRsdoHpH4y9 wHlFRlWkB16iRWQ1Gfflms/94VYKdgsLfrYDN5XgD4+Wkrfc8aEdBYPrdF6onHrD8yXc MmgJG3Zi/TnlbxrWNAM/+cFuCXdt3ma7SzgNKoHJ6MGXYPjKmKw6wzAvI9p5DnFI7xKR l3xQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc; bh=ZWXbwMr5kW7JWgQCKS/f5gIk/1m9Kn3wGidgFrizJ6A=; b=maVaQ8Z78vs3leVMXqLk3e84C6x4MZadr1cPJnCJmsg9LAf6dKP2hClPHC5P44ECpF zZkms5zia0cD0X2bRk5UFTGQa7n09pcn2asmzUiIK3vRCFsfwgqZtcXVRRyLOm4TKKvG cXGfiInvAezlW7WEilzl+c6wmfZgvJVu06Ca2aD1hXYFj+QMAPDdUfB3LBxrqQLNEgId 3albAhiqLJXu7yTG9OuGZm5Cd7LLpc6pKSw4rM78pM90bAE0qWYHZ0TD+Mr5jjq+p8XZ KlplhCBc1eLXXZM5zUN4gHhiR7tHc24WH2se9iEKQSYHAMfRr0obKj72NggQ5ANSnbFk l6xw== X-Gm-Message-State: ACgBeo0PSFeRfNW8jL6jIiZC9SsKpqaWpgWReB5IDpGtZUdH9W6VLzLt 1XYyICJOXr60h0WT6H6GBM398SNouwSsY945 X-Google-Smtp-Source: AA6agR5/jr7ahmdEfbYW1I/9iUbw3X23kb8Jj0Tcd4zyrAewLycRMAMjlLFkBUdKGfgW4dfXClP1eQ== X-Received: by 2002:a05:6214:260c:b0:475:8b1b:9a34 with SMTP id gu12-20020a056214260c00b004758b1b9a34mr334609qvb.85.1660241893303; Thu, 11 Aug 2022 11:18:13 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([199.58.83.12]) by smtp.gmail.com with ESMTPSA id s12-20020a05620a0bcc00b006b5d0b9c5d8sm2498110qki.116.2022.08.11.11.18.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Aug 2022 11:18:13 -0700 (PDT) Date: Thu, 11 Aug 2022 13:18:07 -0500 From: Glenn Washburn To: Josselin Poiret Cc: Daniel Kiper , Daniel Kiper , Patrick Steinhardt , Michael Chang , grub-devel@gnu.org Subject: Re: [PATCH v6 0/2] Have LUKS2 cryptomounts be useable with grub-probe Message-ID: <20220811131807.69e6c47d@crass-HP-ZBook-15-G2> In-Reply-To: References: <20220705110951.ouqnpcfl5q7d5gy7@tomti.i.net-space.pl> Reply-To: development@efficientek.com X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; 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::f33; envelope-from=development@efficientek.com; helo=mail-qv1-xf33.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, 11 Aug 2022 18:18:18 -0000 On Fri, 8 Jul 2022 12:06:06 +0200 Josselin Poiret wrote: > Hello Daniel, > > Thanks for the review. The following updated patches should contain all the changes you asked for. In case this got forgotten, I've reviewed this patch series. The only nit that Daniel can probably fix before committing is to upper case the first word in the added comments. Reviewed-by: Glenn Washburn Glenn > > >Please add your Signed-off-by here. Same applies to first patch too. > Done. > > > Please format comments, here and below, properly [1]. > Sorry about that, I missed the empty first line. Fixed. > > > This function may fail. Please check the returned value and fail if needed. > > [...] > > Ditto. > Fixed (I've used grub_util_error for all the unrecoverable errors, I > hope that's ok). > > >> + name = dm_tree_node_get_name (node); > > I think same applies here... > Right, it can be "", so added a check and comment about the error mode. > > > s/0/NULL/ Please use NULL instead of 0 for pointers. Same below... > Done. > > > grub_strndup() may fail. Please check if cipher != NULL. > Right, sorry. Fixed. > > > seek_head = grub_memchr (c, ' ', remaining); > > if (seek_head == NULL) > Done in both places. > > > Again, grub_strndup() may fail. In general please do not ignore errors > > from functions which may fail. > Done as above. > > Josselin Poiret (2): > devmapper/getroot: Have devmapper recognize LUKS2 > devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM > parameters > > grub-core/osdep/devmapper/getroot.c | 118 ++++++++++++++++++++++++++-- > 1 file changed, 113 insertions(+), 5 deletions(-) > > Range-diff against v5: > 1: 7af629fca = 1: f4cbda414 devmapper/getroot: Have devmapper recognize LUKS2 > 2: 5f9f26118 ! 2: 25ce8bbcc devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters > @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de > lastsubdev, grub_errmsg); > + if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0) > + { > -+ /* set LUKS2 cipher from dm parameters, since it is not > ++ /* > ++ * set LUKS2 cipher from dm parameters, since it is not > + * possible to determine the correct one without > + * unlocking, as there might be multiple segments. > + */ > @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de > + unsigned int remaining; > + > + source = grub_disk_open (grdev); > ++ if (! source) > ++ grub_util_error (_("cannot open grub disk `%s'"), grdev); > + cryptodisk = grub_cryptodisk_get_by_source_disk (source); > ++ if (! cryptodisk) > ++ grub_util_error (_("cannot get cryptodisk from source disk `%s'"), grdev); > + grub_disk_close (source); > + > ++ /* > ++ * the following function always returns a non-NULL pointer, > ++ * but the string may be empty if the relevant info is not present > ++ */ > + name = dm_tree_node_get_name (node); > ++ if (grub_strlen (name) == 0) > ++ grub_util_error (_("cannot get dm node name for grub dev `%s'"), grdev); > + > + grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'", > + uuid, name); > + > + dmt = dm_task_create (DM_DEVICE_TABLE); > -+ if (dmt == 0) > ++ if (dmt == NULL) > + 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 > ++ /* > ++ * 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); > ++ grub_util_error (_("dm target of type `%s' is not `crypt'"), target_type); > + > -+ /* dm target parameters for dm-crypt is > ++ /* > ++ * dm target parameters for dm-crypt is > + * [<#opt_params> ...] > + */ > + c = params; > + remaining = grub_strlen (c); > + > + /* first, get the cipher name from the cipher */ > -+ if (!(seek_head = grub_memchr (c, '-', remaining))) > ++ seek_head = grub_memchr (c, '-', remaining); > ++ if (seek_head == NULL) > + grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"), > + params); > + cipher = grub_strndup (c, seek_head - c); > ++ if (cipher == NULL) > ++ grub_util_error (_("could not strndup cipher of length `%lu'"), seek_head - c); > + remaining -= seek_head - c + 1; > + c = seek_head + 1; > + > + /* now, the cipher mode */ > -+ if (!(seek_head = grub_memchr (c, ' ', remaining))) > ++ seek_head = grub_memchr (c, ' ', remaining); > ++ if (seek_head == NULL) > + grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"), > + params); > + cipher_mode = grub_strndup (c, seek_head - c); > ++ if (cipher_mode == NULL) > ++ grub_util_error (_("could not strndup cipher_mode of length `%lu'"), seek_head - c); > ++ > + remaining -= seek_head - c + 1; > + c = seek_head + 1; > + > @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de > + grub_free (cipher); > + grub_free (cipher_mode); > + > -+ /* This is the only hash usable by PBKDF2, and we don't > ++ /* > ++ * This is the only hash usable by PBKDF2, and we don't > + * have Argon2 support yet, so set it by default, > + * otherwise grub-probe would miss the required > + * abstraction > + */ > + cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256"); > -+ if (cryptodisk->hash == 0) > ++ if (cryptodisk->hash == NULL) > + grub_util_error (_("can't lookup hash sha256 by name")); > + > + dm_task_destroy (dmt); > > base-commit: 351c9c2fd0bcd94c7fda5b697d3283f7f0ff7930