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

[Josselin Poiret]

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, &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
-- 
2.36.1