Re: [PATCH v6 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Fabian Vogt <fvogt@suse.de>]

Hi,

I've played around with LUKS2 using cipher_null and discovered a small issue
with error handling.

Am Freitag, 8. Juli 2022, 12:06:08 CET schrieb Josselin Poiret via Grub-devel:
> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.
> 
> Signed-off-by: Josselin Poiret <dev@jpoiret.xyz>
> ---
>  grub-core/osdep/devmapper/getroot.c | 107 +++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..090ac7167 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);

This change means that in the error paths (grub_util_error), tree is not freed
when exit is called. This should not matter much as the program is about to
exit anyway, but with libdm it does. It registers an exit handler which checks
for leaked objects:

Installing for x86_64-efi platform.
/usr/sbin/grub2-install: error: can't set cipher of cryptodisk `CRYPT-LUKS2-8c0468e27cd4453083107943f126d303-luks' to `cipher_null' with mode `ecb'.
You have a memory leak (not released memory pool):
 [0x562f91463370] dtree
Internal error: Unreleased memory pool(s) found.

This only happens in the failure case so it's just another error message and
not a critical issue.

Cheers,
Fabian

>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -194,7 +195,111 @@ 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 from dm parameters, since it is not
> +               * possible to determine the correct one without
> +               * unlocking, as there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_uint64_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +              char *seek_head, *c;
> +              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 == 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
> +               * 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);
> +
> +              /*
> +               * 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 */
> +              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 */
> +              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;
> +
> +              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +              if (err)
> +                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
> +                                   uuid, cipher, cipher_mode);
> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /*
> +               * 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 == NULL)
> +                  grub_util_error (_("can't lookup hash sha256 by name"));
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else
>