Re: [PATCH v6 1/1] plainmount: Support plain encryption mode

[Glenn Washburn]

I noticed that you've not been including Daniel Kiper on these patches.
I don't expect him to look at these until I'm satisfied with the patch,
but its always good practice to include him. So please do on the next
version of this patch.

On Sun, 31 Jul 2022 17:32:50 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> From 683357e227467c05272facc7da534a82becc9d8a Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sun, 31 Jul 2022 14:06:42 +0100
> Subject: [PATCH v6 1/1] plainmount: Support plain encryption mode
> 
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
> 
> Signed-off-by: Maxim Fomin <maxim@fomin.one>
> ---
>  docs/grub.texi              |  80 +++++++
>  grub-core/Makefile.core.def |   5 +
>  grub-core/disk/plainmount.c | 450 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 535 insertions(+)
>  create mode 100644 grub-core/disk/plainmount.c
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index af119dea3..22c73580c 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4265,6 +4265,7 @@ you forget a command, you can run the command 
> @command{help}
>  * parttool::                    Modify partition table entries
>  * password::                    Set a clear-text password
>  * password_pbkdf2::             Set a hashed password
> +* plainmount::                  Open device encrypted in plain mode
>  * play::                        Play a tune
>  * probe::                       Retrieve device info
>  * rdmsr::                       Read values from model-specific registers
> @@ -4552,6 +4553,14 @@ function is supported, as Argon2 is not yet supported.
> 
>  Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
>  specified without dash separators.
> +
> +Successfully decrypted disks are named as (cryptoX) and have increasing 
> numeration
> +suffix for each new decrypted disk. If the encrypted disk hosts some higher 
> level
> +of abstraction (like LVM2 or MDRAID) it will be created under a separate 
> device
> +namespace in addition to the cryptodisk namespace.
> +
> +Support for plain encryption mode (plain dm-crypt) is provided via separate
> +@command{@pxref{plainmount}} command.
>  @end deffn
> 
>  @node cutmem
> @@ -5060,6 +5069,77 @@ to generate password hashes.  @xref{Security}.
>  @end deffn
> 
> 
> +@node plainmount
> +@subsection plainmount
> +
> +@deffn Command plainmount device @option{-c} cipher @option{-s} key size 
> [@option{-h} hash]
> +[@option{-S} sector size] [@option{-p} password] [@option{-u} uuid]
> +[[@option{-d} keyfile] [@option{-O} keyfile offset]]
> +
> +
> +Setup access to the encrypted device in plain mode. Offset of the encrypted
> +data at the device is specified in terms of 512 byte sectors with the 
> blocklist
> +syntax and loopback device. The following example shows how to specify 1MiB
> +offset:
> +
> +@example
> +loopback node (hd0,gpt1)2048+
> +plainmount node
> +@end example
> +
> +The @command{plainmount} command can be used to open LUKS encrypted volume
> +if its master key and parameters (key size, cipher, offset, etc) are known.
> +
> +There are two ways to specify password: a keyfile and a secret passphrase.

s/password/a password/

> +Keyfile path parameter has higher priority than secret passphrase and is

s/Keyfile/The keyfile/
s/secret passphrase/the secret passphrase parameter/

> +specified with the option @option{-d}. Password data obtained from keyfiles
> +is not hashed and is used directly as a cipher key. Optional offset of 
> password

s/Optional/An optional/

> +data in the keyfile can be specified with the option @option{-O} or directly
> +with the option @option{-d} and GRUB blocklist syntax. The following example
> +shows both methods to specify password data in the keyfile at offset 1MiB:
> +
> +@example
> +plainmount -d (hd0,gpt1)2048+
> +plainmount -d (hd0,gpt1) -O 1048576

I don't think this will work. I think it has to be something like:

  plainmount -d (hd0,gpt1)+ -O 1048576

> +@end example
> +
> +If no keyfile is specified then the password is set to data specified by

s/data/the string/

> +option @option{-p} or is requested interactively from the console. In both
> +cases the provided password is hashed with the algorithm specified by the
> +option @option{-h}. This option is mandatory if no keyfile is specified, but
> +it can be set to @code{plain} which means that no hashing is done and such

I was mistaken in my last suggestion to use @code here, instead @samp
should be used because this is sample input.

> +password is used directly as a key.
> +
> +Cipher @option{-c} and keysize @option{-s} options specify the cipher
> +algorithm and the key size respectively and are mandatory options. Cipher
> +must be specified with the mode separated by a dash (for example,
> +'aes-xts-plain64'). Key size option @option{-s} is the key size of the 
> cipher,

s/'aes-xts-plain64'/@samp{aes-xts-plain64}/
s/cipher/cipher in bits/

> +not to be confused with the offset of the key data in a keyfile specified 
> with
> +the @option{-O} option. It must not exceed 128 bytes and must be specified in
> +bits (so a 32 byte key would be specified as 256 bits).

Also, I'm seeing now that we need not talk about the maximum number of
bytes here since the user is inputting in bits, so I think this
sentence is better:

  It must not exceed 1024 bits, so a 32 byte key would be specified as
  256 bits.

> +
> +The optional parameter @option{-S} specifies encrypted device sector size. It
> +must be at least 512 bytes long (default value) and a power of 2. 
> @footnote{Current
> +implementation of cryptsetup supports only 512/1024/2048/4096 byte sectors}.
> +Disk sector size is configured when creating the encrypted volume. Attempting
> +to decrypt volumes with a different sector size than it was created with will
> +not result in an error, but will decrypt to random bytes and thus prevent
> +accessing the volume (in some cases filesystem driver can detect the 
> filesystem

s/filesystem driver/the filesystem driver/

s/filesystem presense/presence of a filesystem/

> +presense, but nevertheless will refuse to mount it).
> +
> +By default new plainmount devices will be given a UUID starting with
> +'109fea84-a6b7-34a8-4bd1-1c506305a401' where the last digits are incremented
> +by one for each plainmounted device beyond the first up to 2^10 devices.
> +
> +All encryption arguments (cipher, hash, key size, disk offset and disk sector
> +size) must match the parameters used to create the volume. If any of them 
> does
> +not match the actual arguments used during the initial encryption, plainmount
> +will create virtual device with the garbage data and GRUB will report unknown
> +filesystem for such device. Writing data to such virtual device will result 
> in
> +the data loss if the underlying partition contained desired data.
> +@end deffn
> +
> +
>  @node play
>  @subsection play
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 715994872..3910b7670 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1174,6 +1174,11 @@ module = {
>    common = disk/cryptodisk.c;
>  };
> 
> +module = {
> +  name = plainmount;
> +  common = disk/plainmount.c;
> +};
> +
>  module = {
>    name = json;
>    common = lib/json/json.c;
> diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
> new file mode 100644
> index 000000000..8b735a425
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,450 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/* plaimount.c - Open device encrypted in plain mode. */
> +
> +#include <grub/cryptodisk.h>
> +#include <grub/dl.h>
> +#include <grub/err.h>
> +#include <grub/extcmd.h>
> +#include <grub/partition.h>
> +#include <grub/file.h>
> +
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
> +#define PLAINMOUNT_DEFAULT_UUID        "109fea84-a6b7-34a8-4bd1-1c506305a400"
> +
> +
> +enum PLAINMOUNT_OPTION
> +  {
> +    OPTION_HASH,
> +    OPTION_CIPHER,
> +    OPTION_KEY_SIZE,
> +    OPTION_SECTOR_SIZE,
> +    OPTION_PASSWORD,
> +    OPTION_KEYFILE,
> +    OPTION_KEYFILE_OFFSET,
> +    OPTION_UUID
> +  };
> +
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    /* TRANSLATORS: It's still restricted to this module only.  */
> +    {"hash", 'h', 0, N_("Password hash"), 0, ARG_TYPE_STRING},
> +    {"cipher", 'c', 0, N_("Password cipher"), 0, ARG_TYPE_STRING},
> +    {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
> +    {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
> +    {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
> +    {"keyfile", 'd', 0, N_("Keyfile path"), 0, ARG_TYPE_STRING},
> +    {"keyfile-offset", 'O', 0, N_("Keyfile offset"), 0, ARG_TYPE_INT},
> +    {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +
> +/* Cryptodisk setkey() function wrapper */
> +static grub_err_t
> +plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *key,
> +                grub_size_t size)
> +{
> +  gcry_err_code_t code = grub_cryptodisk_setkey (dev, key, size);
> +  if (code != GPG_ERR_NO_ERROR)
> +    {
> +      grub_dprintf ("plainmount", "failed to set cipher key with code: 
> %d\n", code);
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot set specified 
> key"));
> +    }
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Configure cryptodisk uuid */
> +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char 
> *user_uuid)
> +{
> +  grub_size_t pos = 0;
> +
> +  /* Size of user_uuid is checked in main func */
> +  if (user_uuid != NULL)
> +      grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> +  else
> +    {
> +      /*
> +       * Set default UUID. Last digits start from 1 and are incremented for
> +       * each new plainmount device by snprintf().
> +       */
> +      grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
> +      while (dev->uuid[++pos] == ' ');
> +      grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos);
> +    }
> +  COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof 
> (PLAINMOUNT_DEFAULT_UUID));
> +}
> +
> +
> +/* Configure cryptodevice sector size (-S option) */
> +static grub_err_t
> +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t disk,
> +                           grub_size_t sector_size)
> +{
> +  dev->total_sectors = grub_disk_native_sectors (disk);
> +  if (dev->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> +    return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot determine disk %s 
> size"),
> +                    disk->name);
> +
> +  /* Convert size to sectors */
> +  dev->log_sector_size = grub_log2ull (sector_size);
> +  dev->total_sectors = grub_convert_sector (dev->total_sectors,
> +                                         GRUB_DISK_SECTOR_BITS,
> +                                         dev->log_sector_size);
> +  if (dev->total_sectors == 0)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +                    N_("cannot set specified sector size on disk %s"),
> +                    disk->name);
> +
> +  grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"
> +             PRIuGRUB_SIZE"\n", dev->log_sector_size, dev->total_sectors);
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Hashes a password into a key and stores it with cipher. */
> +static grub_uint8_t*
> +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> +                            grub_uint8_t *key_data, grub_size_t key_size)

Why does the return type changed from v5? I like it was better before,
and I'm thinking the signature should be more like hash() in
cryptsetup, that is have password and password_size arguments, to get
rid of the non-NULL byte assumption. Moving the password asking code
out of this function is fine though.

> +{
> +  grub_uint8_t *derived_hash, *dh;
> +  char *p;
> +  unsigned int round, i;
> +  unsigned int len, size;
> +
> +  /* Support none (plain) hash */
> +  if (grub_strcmp (hash, "plain") == 0)
> +    {
> +      dev->hash = NULL;
> +      return key_data;
> +    }
> +
> +  /* Hash argument was checked at main func */

It was? I'm not seeing the check below...

> +  dev->hash = grub_crypto_lookup_md_by_name (hash);
> +  len = dev->hash->mdlen;
> +  p = grub_malloc (key_size + 2 + (key_size / len));

This should password_size + (key_size / len). I forget what the 2 above
should be from (NULL byte and something else?), but I'm not sure its
needed. The hash() function in cryptsetup allows for NULL bytes in the
password string. I think we should also, so p doesn't need to be NULL
terminated. 

> +  derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> +  if (p == NULL || derived_hash == NULL)
> +    {
> +      grub_free (p);
> +      grub_free (derived_hash);
> +      grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      return NULL;
> +    }
> +  dh = derived_hash;
> +
> +  /*
> +   * Hash password. Adapted from cryptsetup.
> +   * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> +   */
> +  for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> +    {
> +      for (i = 0; i < round; i++)
> +     p[i] = 'A';
> +
> +      grub_strcpy (p + i, (char*) key_data);

This assumes that there are no NULL bytes in key_data.

> +
> +      if (len > size)
> +     len = size;
> +
> +      grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));

This also has a non-NULL byte assumption.

> +    }
> +  grub_free (p);
> +  return derived_hash;
> +}
> +
> +
> +/* Read key material from keyfile */
> +static grub_err_t
> +plainmount_configure_keyfile (char *keyfile, grub_uint8_t *key_data,
> +                           grub_size_t key_size, grub_size_t keyfile_offset)
> +{
> +  grub_file_t g_keyfile = grub_file_open (keyfile, GRUB_FILE_TYPE_NONE);
> +  if (g_keyfile == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
> +                    keyfile);
> +
> +  if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1)
> +    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +                    N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
> +                    keyfile_offset);
> +
> +  if (key_size > (g_keyfile->size - keyfile_offset))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Specified key size (%"
> +                    PRIuGRUB_SIZE") is too small for keyfile size (%"
> +                    PRIuGRUB_SIZE") and offset (%"PRIuGRUB_SIZE")"),
> +                    key_size, g_keyfile->size, keyfile_offset);
> +
> +  if (grub_file_read (g_keyfile, key_data, key_size) != (grub_ssize_t) 
> key_size)
> +    return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key 
> file"));
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Plainmount command entry point */
> +static grub_err_t
> +grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char **args)
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +  grub_cryptodisk_t dev = NULL;
> +  grub_disk_t disk = NULL;
> +  const gcry_md_spec_t *gcry_hash;
> +  char *diskname, *disklast = NULL, *cipher, *mode, *hash, *keyfile, *uuid;
> +  grub_size_t len, key_size, sector_size, keyfile_offset = 0;
> +  grub_err_t err;
> +  const char *p;
> +  grub_uint8_t *key_data;
> +  grub_uint8_t *hashed_key_data;
> +
> +  if (argc < 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> +
> +  /* Check whether required arguments are specified */
> +  if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key size must be 
> set");
> +  if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm must be 
> set");
> +
> +  /* Check hash */
> +  if (!state[OPTION_KEYFILE].set)
> +  {
> +    gcry_hash = grub_crypto_lookup_md_by_name (state[OPTION_HASH].arg);
> +    if (!gcry_hash)
> +      return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("couldn't load hash 
> %s"),
> +                      state[OPTION_HASH].arg);
> +
> +    if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                      N_("hash length %"PRIuGRUB_SIZE" exceeds maximum %d 
> bits"),
> +                      gcry_hash->mdlen * GRUB_CHAR_BIT,
> +                      GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
> +   }
> +
> +  /* Check cipher mode */
> +  if (!grub_strchr (state[OPTION_CIPHER].arg,'-'))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("invalid cipher mode, must be of format 
> cipher-mode"));
> +
> +  /* Check password size */
> +  if (state[OPTION_PASSWORD].set && grub_strlen (state[OPTION_PASSWORD].arg) 
> >
> +                                              GRUB_CRYPTODISK_MAX_PASSPHRASE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("password exceeds maximium size"));
> +
> +  /* Check uuid length */
> +  if (state[OPTION_UUID].set && grub_strlen (state[OPTION_UUID].arg) >
> +                             sizeof (PLAINMOUNT_DEFAULT_UUID))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("specified UUID exceeds maximum size"));
> +
> +  /* Parse plainmount arguments */
> +  grub_errno = GRUB_ERR_NONE;
> +  keyfile_offset = state[OPTION_KEYFILE_OFFSET].set ?
> +                grub_strtoull (state[OPTION_KEYFILE_OFFSET].arg, &p, 0) : 0;
> +  if (state[OPTION_KEYFILE_OFFSET].set &&
> +     (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0' ||
> +      grub_errno != GRUB_ERR_NONE))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized keyfile 
> offset"));
> +
> +  sector_size = state[OPTION_SECTOR_SIZE].set ?
> +             grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, 0) :
> +             PLAINMOUNT_DEFAULT_SECTOR_SIZE;
> +  if (state[OPTION_SECTOR_SIZE].set && (state[OPTION_SECTOR_SIZE].arg[0] == 
> '\0' ||
> +                                     *p != '\0' || grub_errno != 
> GRUB_ERR_NONE))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector 
> size"));
> +
> +  /* Check key size */
> +  key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0);
> +  if (state[OPTION_KEY_SIZE].arg[0] == '\0' || *p != '\0' ||
> +      grub_errno != GRUB_ERR_NONE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
> +  if (key_size % GRUB_CHAR_BIT != 0)

I think parenthesis around "key_size % GRUB_CHAR_BIT" would be good.

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("key size is not multiple of %d bits"), 
> GRUB_CHAR_BIT);
> +  key_size = key_size / GRUB_CHAR_BIT;
> +  if (key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("key size %"PRIuGRUB_SIZE" exceeds maximum %d bits"),
> +                    key_size * GRUB_CHAR_BIT,
> +                    GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT);
> +
> +  /* Check disk sector size */
> +  if (sector_size < GRUB_DISK_SECTOR_SIZE)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("sector size -S must be at least %d"),
> +                    GRUB_DISK_SECTOR_SIZE);
> +  if ((sector_size & (sector_size - 1)) != 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                    N_("sector size -S %"PRIuGRUB_SIZE" is not power of 2"),
> +                    sector_size);
> +
> +  /* Allocate all stuff here */
> +  hash =  state[OPTION_HASH].set ? grub_strdup (state[OPTION_HASH].arg) : 
> NULL;
> +  cipher = grub_strdup (state[OPTION_CIPHER].arg);
> +  keyfile = state[OPTION_KEYFILE].set ?
> +         grub_strdup (state[OPTION_KEYFILE].arg) : NULL;
> +  dev = grub_zalloc (sizeof *dev);
> +  key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +  uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) : 
> NULL;
> +  if ((hash == NULL && state[OPTION_HASH].set) || cipher == NULL || dev == 
> NULL ||
> +      (keyfile == NULL && state[OPTION_KEYFILE].set) || key_data == NULL ||
> +      (uuid == NULL && state[OPTION_UUID].set))
> +    {
> +      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      goto exit;
> +    }
> +
> +  /* Copy user password from -p option */
> +  if (state[OPTION_PASSWORD].set)
> +    grub_memcpy (key_data, state[OPTION_PASSWORD].arg,
> +              grub_strlen (state[OPTION_PASSWORD].arg));
> +
> +  /* Copy user UUID from -u option */
> +  if (state[OPTION_UUID].set)
> +    grub_memcpy (uuid, state[OPTION_UUID].arg,
> +              grub_strlen (state[OPTION_UUID].arg));
> +
> +  /* Set cipher mode (tested above) */
> +  mode = grub_strchr (cipher,'-');
> +  *mode++ = '\0';
> +
> +  /* Check cipher */
> +  if (grub_cryptodisk_setcipher (dev, cipher, mode) != GRUB_ERR_NONE)
> +    {
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher %s"), 
> cipher);
> +      goto exit;
> +    }
> +
> +  /* Open SOURCE disk */
> +  diskname = args[0];
> +  len = grub_strlen (diskname);
> +  if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> +    {
> +      disklast = &diskname[len - 1];
> +      *disklast = '\0';
> +      diskname++;
> +    }
> +  disk = grub_disk_open (diskname);
> +  if (disk == NULL)
> +    {
> +      if (disklast)
> +        *disklast = ')';
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot open disk %s"), 
> diskname);
> +      goto exit;
> +    }
> +
> +  /* Get password from console */
> +  if (keyfile == NULL && key_data[0] == '\0')
> +  {
> +    char *part = grub_partition_get_name (disk->partition);
> +    grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name,
> +               disk->partition != NULL ? "," : "",
> +               part != NULL ? part : N_("UNKNOWN"));
> +    grub_free (part);
> +
> +    if (!grub_password_get ((char*)key_data, 
> GRUB_CRYPTODISK_MAX_PASSPHRASE-1))
> +        grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading password"));
> +  }
> +
> +  /* Warn if hash and keyfile are both provided */
> +  if (keyfile != NULL && state[OPTION_HASH].arg)

Seems inconsistent to use "keyfile != NULL" instead of
state[OPTION_KEYFILE].set, like the ifs below. Is there a reason for
this that I'm not thinking of?

> +    grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> +
> +  /* Warn if -p option is specified with keyfile */
> +  if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> +    grub_printf_ (N_("warning: password specified with -p option "
> +                  "is ignored if keyfile is provided\n"));
> +
> +  /* Warn of -O is provided without keyfile */
> +  if (state[OPTION_KEYFILE_OFFSET].set && !state[OPTION_KEYFILE].set)
> +    grub_printf_ (N_("warning: keyfile offset option -O "
> +                  "specified without keyfile option -d\n"));
> +
> +  grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, key_size=%"
> +             PRIuGRUB_SIZE", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> +             cipher, hash, key_size, keyfile, keyfile_offset);
> +
> +  err = plainmount_configure_sectors (dev, disk, sector_size);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  /* Configure keyfile or password */
> +  if (keyfile != NULL)
> +    {
> +      err = plainmount_configure_keyfile (keyfile, key_data, key_size, 
> keyfile_offset);
> +      if (err != GRUB_ERR_NONE)
> +        goto exit;
> +      err = plainmount_setkey (dev, key_data, key_size);
> +      if (err != GRUB_ERR_NONE)
> +        goto exit;
> +    }
> +  else
> +    {
> +      hashed_key_data = plainmount_configure_password (dev, hash, key_data, 
> key_size);

It looks like you're limiting key_data, which could be a string from
-p, to key_size. The cryptsetup code does not appear to do this, so I
think this does not work for passwords longer than the hash length.

> +      if (hashed_key_data == NULL)
> +        goto exit;
> +      err = plainmount_setkey (dev, hashed_key_data, key_size);
> +      if (err != GRUB_ERR_NONE)
> +      {
> +        grub_free (hashed_key_data);
> +        goto exit;
> +      }
> +    }

I was hoping that when moving plainmount_setkey() out of the
plainmount_configure_*() functions that it could be only called once in
the code, instead of twice as done here. Why can't we refactor and have
this code here:

  err = plainmount_setkey (dev, key_data, key_size);
  if (err != GRUB_ERR_NONE)
    goto exit;

Glenn

> +
> +  err = grub_cryptodisk_insert (dev, diskname, disk);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  dev->modname = "plainmount";
> +  dev->source_disk = disk;
> +  plainmount_set_uuid (dev, uuid);
> +
> +exit:
> +  grub_free (hash);
> +  grub_free (cipher);
> +  grub_free (keyfile);
> +  grub_free (key_data);
> +  grub_free (uuid);
> +  if (err != GRUB_ERR_NONE && disk)
> +    grub_disk_close (disk);
> +  if (err != GRUB_ERR_NONE && dev)
> +    grub_free (dev);
> +  return err;
> +}
> +
> +static grub_extcmd_t cmd;
> +GRUB_MOD_INIT (plainmount)
> +{
> +  cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> +                           N_("-c cipher -s key-size [-h hash] [-S 
> sector-size]"
> +                           " [-o offset] [-p password] [-u uuid] "
> +                           " [[-d keyfile] [-O keyfile offset]] <SOURCE>"),
> +                           N_("Open partition encrypted in plain mode."),
> +                           options);
> +}
> +
> +GRUB_MOD_FINI (plainmount)
> +{
> +  grub_unregister_extcmd (cmd);
> +}
> --
> 2.37.1
> 
>