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

[Daniel Kiper]

First of all, sorry for late reply. I hope I will be able to review next
version of this patch much faster.

On Sat, Oct 29, 2022 at 05:40:42PM +0000, Maxim Fomin wrote:
> From 2b1d2deb3f2416cbc3e7d25cbc4141a3078eaf68 Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sat, 29 Oct 2022 18:18:58 +0100
> Subject: [PATCH v8 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>

Please put "---" behind SOB and before "Difference with..." next time...

> Difference with v7:

[...]

> diff --git a/docs/grub.texi b/docs/grub.texi
> index 2d6cd8358..34ca6b4f1 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4271,6 +4271,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
> @@ -4558,6 +4559,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
> @@ -5120,6 +5129,78 @@ 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 using the 
> blocklist
> +syntax and loopback device. The following example shows how to specify 1MiB
> +offset:
> +
> +@example
> +loopback node (hd0,gpt1)2048+
> +plainmount node @var{...}
> +@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 a password: a keyfile and a secret passphrase.
> +The keyfile path parameter has higher priority than the secret passphrase
> +parameter and is specified with the option @option{-d}. Password data 
> obtained
> +from keyfiles is not hashed and is used directly as a cipher key. An optional
> +offset of password data in the keyfile can be specified with the option
> +@option{-O} or directly with the option @option{-d} and GRUB blocklist 
> syntax,
> +if the keyfile data can be accessed from a device and is 512 byte aligned.
> +The following example shows both methods to specify password data in the
> +keyfile at offset 1MiB:
> +
> +@example
> +plainmount -d (hd0,gpt1)2048+ @var{...}
> +plainmount -d (hd0,gpt1)+ -O 1048576 @var{...}
> +@end example
> +
> +If no keyfile is specified then the password is set to the string specified
> +by 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 @samp{plain} which means that no hashing is done and such
> +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, @samp{aes-xts-plain64}). Key 
> size
> +option @option{-s} is the key size of the 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 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 the filesystem driver can detect the 
> presence
> +of a filesystem, 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.

Hmmm... Why do you say "Writing data" here if the GRUB does not support
filesystems writes except grubenv file? I think it should be clarified
what you mean or dropped if it is wrong in this context. Otherwise it is
confusing.

> +@end deffn
> +
> +
>  @node play
>  @subsection play
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 98714c68d..f4153608c 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1184,6 +1184,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..85ada25bc
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,462 @@
> +/*
> + *  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>
> +
> +

Please use one line instead of two to separate things. Same comment
applies to double empty lines below.

> +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));

I think grub_strcpy() instead of grub_memcpy() would be more natural in
string contexts. And then you do not need grub_strlen().

Is it safe to assume here that dev->uuid has been zeroed earlier?

> +  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);

s/sizeof (dev->uuid)-1/sizeof (dev->uuid) - 1/
s/dev->id+1/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 the cipher. */
> +static grub_err_t
> +plainmount_configure_password (grub_cryptodisk_t dev, const char *hash,
> +                            grub_uint8_t *key_data, grub_size_t key_size,
> +                            grub_size_t password_size)
> +{
> +  grub_uint8_t *derived_hash, *dh;
> +  char *p;
> +  unsigned int round, i, len, size;
> +  grub_size_t alloc_size;
> +  grub_err_t err = GRUB_ERR_NONE;
> +
> +  /* Support none (plain) hash */
> +  if (grub_strcmp (hash, "plain") == 0)
> +    {
> +      dev->hash = NULL;
> +      return err;
> +    }
> +
> +  /* Hash argument was checked at main func */
> +  dev->hash = grub_crypto_lookup_md_by_name (hash);
> +  len = dev->hash->mdlen;
> +
> +  alloc_size = grub_max (password_size, key_size);
> +  /*
> +   * Allocate buffer for the password and for an added prefix character
> +   * for each hash round ('alloc_size' may not be a multiple of 'len').
> +   */
> +  p = grub_zalloc (alloc_size + (alloc_size / len) + 1);
> +  derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> +  if (p == NULL || derived_hash == NULL)
> +    {
> +      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      goto exit;
> +    }
> +  dh = derived_hash;
> +
> +  /*
> +   * Hash password. Adapted from cryptsetup.
> +   * https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> +   */
> +  for (round = 0, size = alloc_size; size; round++, dh += len, size -= len)
> +    {
> +      for (i = 0; i < round; i++)
> +     p[i] = 'A';
> +
> +      grub_memcpy (p + i, (char*) key_data, password_size);
> +
> +      if (len > size)
> +     len = size;
> +
> +      grub_crypto_hash (dev->hash, dh, p, password_size + round);
> +    }
> +  grub_memcpy (key_data, derived_hash, key_size);
> +
> +exit:

I prefer "fail" instead of "exit" label in that context. And labels
should be prefixed with one space. Same applies to "exit" labels below.

> +  grub_free (p);
> +  grub_free (derived_hash);
> +  return err;
> +}
> +
> +
> +/* 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)

s/(grub_off_t)-1/(grub_off_t) -1/
Casts require a space after ")"...

> +    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, password_size 
> = 0;
> +  grub_err_t err;
> +  const char *p;
> +  grub_uint8_t *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"));

What will happen if somebody passes shorter, probably non-valid, UUID?
I think we should check UUID is not shorter than something too.

> +  /* 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)
> +    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 ||

I think it would be more natural if you do "(state[OPTION_HASH].set && hash == 
NULL)..."
instead of "(hash == NULL && state[OPTION_HASH].set)...".

> +      (keyfile == NULL && state[OPTION_KEYFILE].set) || key_data == NULL ||

Ditto.

> +      (uuid == NULL && state[OPTION_UUID].set))

Ditto.

> +    {
> +      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> +      goto exit;
> +    }
> +
> +  /* Copy user password from -p option */
> +  if (state[OPTION_PASSWORD].set)
> +    {
> +      /*
> +       * Password from the '-p' option is limited to C-string.
> +       * Arbitrary data keys are supported via keyfiles.
> +       */
> +      password_size = grub_strlen (state[OPTION_PASSWORD].arg);
> +      grub_memcpy (key_data, state[OPTION_PASSWORD].arg, password_size);
> +    }
> +
> +  /* Copy user UUID from -u option */
> +  if (state[OPTION_UUID].set)
> +    grub_memcpy (uuid, state[OPTION_UUID].arg,
> +              grub_strlen (state[OPTION_UUID].arg));

This seems duplicate. grub_strdup() did work for you above.

> +
> +  /* 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 (!state[OPTION_KEYFILE].set && 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))

s/(char*)key_data/(char *) key_data/
s/GRUB_CRYPTODISK_MAX_PASSPHRASE-1/GRUB_CRYPTODISK_MAX_PASSPHRASE - 1/

> +      {
> +        err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading 
> password"));
> +        goto exit;
> +      }
> +    /*
> +     * Password from interactive console is limited to C-string.
> +     * Arbitrary data keys are supported via keyfiles.
> +     */
> +    password_size = grub_strlen (key_data);
> +  }
> +
> +  /* Warn if hash and keyfile are both provided */
> +  if (state[OPTION_KEYFILE].set && state[OPTION_HASH].arg)
> +    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",

s/"PRIuGRUB_SIZE"/" PRIuGRUB_SIZE "/

> +             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 (state[OPTION_KEYFILE].set)
> +    err = plainmount_configure_keyfile (keyfile, key_data, key_size, 
> keyfile_offset);
> +  else
> +    err = plainmount_configure_password (dev, hash, key_data, key_size, 
> password_size);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  err = plainmount_setkey (dev, key_data, key_size);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  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)

s/disk/disk != NULL/

> +    grub_disk_close (disk);
> +  if (err != GRUB_ERR_NONE && dev)

You can drop "dev" check from here.

> +    grub_free (dev);
> +  return err;
> +}

Daniel