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

[Glenn Washburn]

Thanks for making these updates.

On Fri, 20 May 2022 20:20:39 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
> filled out, otherwise they wouldn't be initialized if cheat mounted.
> ---
>  grub-core/osdep/devmapper/getroot.c | 130 +++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c 
> b/grub-core/osdep/devmapper/getroot.c
> index ad1daf9c8..b8d66029a 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -43,6 +43,8 @@
>  #include <grub/osdep/major.h>
>  #include <libdevmapper.h>
>  
> +#define DM_LOG_SECTOR_SIZE 9
> +
>  #include <grub/types.h>
>  #include <grub/util/misc.h>
>  
> @@ -51,6 +53,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)
> @@ -183,7 +187,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>       {
>         grub_err_t err;
> @@ -191,7 +194,132 @@ 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 and size from dm parameters, since it is 
> not
> +               * possible to determine the correct ones without unlocking, as
> +               * there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_addr_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +
> +              source = grub_disk_open (grdev);
> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              grub_disk_close (source);
> +
> +              name = dm_tree_node_get_name (node);
> +
> +              grub_util_info ("populating parameters of cryptomount `%s' 
> from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == 0)
> +                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);
> +
> +              cryptodisk->total_sectors = length;
> +              cryptodisk->log_sector_size = DM_LOG_SECTOR_SIZE;
> +
> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> 
> [<#opt_params> <opt_param1> ...]
> +               */
> +              char *seek_head, *c;
> +              unsigned int remaining;

Daniel might want these declared up with the rest at the start of the
block.

> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', remaining)))
> +                grub_util_error (_("can't get cipher from dm-crypt 
> parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c;

I think remaining should technically be one less here because of the
line below so that it can be used in the following grub_memchr() as is.

> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', remaining)))
> +                grub_util_error (_("can't get cipher mode from dm-crypt 
> parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c;

Ditto. Now we're off by 2 after the next line.

> +              c = seek_head + 1;
> +
> +              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);

Probably should error check the return here.

> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, so set it as such */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");

This should do an error check for if (cryptodisk->hash == NULL).

My prior review had a comment here on if the hash might change if/when
argon2 is supported. I'm thinking that we can visit that when argon2
support gets added. So I'm fine with having hash be a constant.

> +
> +              /* drop key, iv_offset, device path and offset */
> +              for (char dropped_tokens = 0; dropped_tokens < 4; 
> dropped_tokens++)

Why did dropped_tokens get changed to a "char"? Does it really buy us
anything?

> +                {

Probably should check for (seek_head == NULL), just in case. Perhaps if
true do: 
  grub_util_error (_("dm-crypt parameters missing required fields
`%s'"), params);

I'm suggesting this above the grub_memchr() so that we don't check the
last iteration of the loop, in which seek_head == NULL is not an error.
If you want to restructure this some other way, go ahead.

> +                  seek_head = grub_memchr (c, ' ', remaining);
> +                  remaining -= seek_head - c;

Ditto, on an off-by-one error here. These will accumulate such that
grub_memchr() may return a position outside the bounds of the params
string.

> +                  c = seek_head + 1;
> +                }
> +
> +              /* if we have optional parameters, skip #opt_params */

This comment doesn't quite make sense to me. What is #opt_params? I
presume "number of optional parameters", but the code doesn't skip
optional parameters. It searches through the optional parameters for
sector_size, and once it finds it, then breaks out. It seems like it
should be something like:

/* if we have optional parameters, search for sector_size */

This comment would still be good even if getting rid of the if block
enclosing the while loop as suggested below.

> +              if (seek_head && (seek_head = grub_memchr (c, ' ', remaining)))
> +                {
> +                  remaining -= seek_head - c;
> +                  c = seek_head + 1;

I still don't see the utility of these lines above, along with the "if"
statement. You should be able to remove them, thus having the while
outside of this block, and still the code would be correct.

The check for seek_head != NULL can be done in the while condition. If
there are optional parameters, then seek_head should be non-NULL and
"c" should point to the first character of the first listed optional
parameter, assuming all parameters are separated by a single space.
Even if that is not true and there are multiple spaces between some
parameters, the while loop will handle it fine, just perhaps not as
efficiently as it could.

> +                  while (remaining > 0)
> +                    {
> +                      if (strncmp (c, "sector_size:", sizeof 
> ("sector_size:") - 1) == 0)

If we're concerned about multiple spaces between parameters, we could
make this more efficient by using this conditional instead:

if (*c != ' ' && strncmp (c, "sector_size:", sizeof ("sector_size:") -
1) == 0)

This way the strncmp only runs if the character c points to is not a
space. I'm not really concerned about extra spaces though and am fine
with the inefficiency if there are.

> +                        {
> +                          char *sector_size_str;
> +                          unsigned long long sector_size;
> +                          c += sizeof ("sector_size:") - 1;
> +                          remaining -= sizeof ("sector_size:") - 1;
> +                          seek_head = grub_memchr (c, ' ', remaining);
> +                          if (seek_head)
> +                            sector_size_str = grub_strndup (c, seek_head - 
> c);
> +                          else
> +                            sector_size_str = grub_strndup (c, remaining);
> +                          sector_size = grub_strtoull (sector_size_str, 
> NULL, 10);

This 

> +                          grub_free (sector_size_str);
> +
> +                          if (sector_size != 512 && sector_size != 1024 &&
> +                              sector_size != 2048 && sector_size != 4096)
> +                            grub_util_error (_("dm-crypt parameter 
> sector_size `%s' is not a valid LUKS2 sector size"),
> +                                             sector_size_str);
> +                          cryptodisk->log_sector_size = grub_log2ull 
> (sector_size);
> +                          cryptodisk->total_sectors = 
> cryptodisk->total_sectors /
> +                            (sector_size / (1 << DM_LOG_SECTOR_SIZE));

Should use grub_convert_sector() in include/grub/disk.h, eg:

 cryptodisk->total_sectors = grub_convert_sector
 (cryptodisk->total_sectors,
      cryptodisk->log_sector_size, DM_LOG_SECTOR_SIZE);

> +                          grub_util_info ("set sector_size for cryptodisk 
> `%s' to `%d', total_sectors to `%lu'",
> +                                          name, 1 << 
> cryptodisk->log_sector_size,

could use sector_size instead of 1 << cryptodisk->log_sector_size

> +                                          cryptodisk->total_sectors);
> +                          break;
> +                        }
> +                      seek_head = grub_memchr (c, ' ', remaining);
> +                      remaining -= seek_head - c;

If none of the optional parameters start with "sector_size:" and
remaining is correct, then we'll come to the last optional parameter
and grub_memchr() will return NULL. Then remaining will be (remaining -
( 0 - c )) or (remaining + c), where c is likely a large number. Then
the while condition, remaining > 0, will continue to be true and go
through another iteration. But onthis next iteration c will be pointing
to address 1 because of the line below. Accessing *c at that point
might cause a crash or later on as the train has been derailed.

> +                      c = seek_head + 1;
> +                    }
> +                }
> +
> +              dm_task_destroy (dmt);
> +            }
>       }
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else

After reviewing this, I stumbled upon Fabian's patch. I wrote a
response on the thread of that patch with the suggestion that his patch
be used to get the total number of sectors and sector size and this
patch be used to get the crypto information. This would obviate the
problematic parsing part of this patch. What do you think about
removing all the parsing code after the setting of cryptodisk->hash?

Glenn