Hello Daniel,
Thanks for the review. The following updated patches should contain all the
changes you asked for.
>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
+ * <cipher> <key> <iv_offset> <device path> <offset>
[<#opt_params> <opt_param1> ...]
+ */
+ 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
--
2.36.1
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
