Re: [PATCH v6 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Glenn Washburn]

On Fri,  8 Jul 2022 12:06:06 +0200
Josselin Poiret <dev@jpoiret.xyz> 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 <development@efficientek.com>

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, &params);
>      +              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