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

[Glenn Washburn]

On Sat, 18 Jun 2022 14:33:48 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> From 87af7e9cbeb72c3bc146564f64aa8132c1bf6d68 Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sat, 18 Jun 2022 14:32:49 +0100
> Subject: [PATCH v3 1/1] plainmount: Support plain encryption mode.
> 
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
> 

There should be a line with only "---" here so that the Changes lines
do not get added as part of the commit message.

> Changes from the second version of the patch:
> - added support for blocklist syntax offset
> - added explicit error for unsupported offset syntax
> - removed offset options -o and -O

I think it makes sense to keep -O because that's the only way to do a
byte offset into the keyfile (without the special "blocklist" parsing),
just as in cryptomount.

> - removed disk size -b option
> - added static compile-time uuid
> - removed 4096 sector size limitation
> - parameters are passed directly (not inside struct)
> - added support for plain hash
> - minor code improvements
> - improved documentation
> ---
>  docs/grub.texi              |  54 ++++
>  grub-core/Makefile.core.def |   5 +
>  grub-core/disk/plainmount.c | 517 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 576 insertions(+)
>  create mode 100644 grub-core/disk/plainmount.c
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 9b902273c..a72475a8b 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4229,6 +4229,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
> @@ -4509,6 +4510,9 @@ 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.
> +
> +Support for plain encryption mode (plain dm-crypt) is provided via separate
> +plainmount command.
>  @end deffn
> 
>  @node cutmem
> @@ -5017,6 +5021,56 @@ 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{-d} keyfile] 
> [@option{-u} uuid]
> +
> +Setup access to encrypted in plain mode device. Password can be specified in

"Setup access to encrypted device in plain mode."

> +two ways: as a secret passphrase or as a byte array from a keyfile. Secret

"keyfile. The secret passphrase"

There are a quite a few articles (eg. "the" or "a") missing here. I've
not listed them all. This is not a criticism, as this is in general
well written, but I'm noting it so we can fix it if desired. I don't
think it makes anything confusing as is, it just is slightly noticeable
that its written by a non-native speaker. So I don't mind it as is.

> +passphrase can be provided interactively from console or with option 
> @option{-p}.

"the option"

> +The keyfile is specified with option @option{-d} and can be either a path to 
> a
> +regular file or a block device. The keyfile parameter @option{-d} has higher

Take out "or a block device"

> +priority than option @option{-p}.
> +
> +Cipher @option{-c} and keysize @option{-s} options are mandatory. Hash option
> +@option{-h} is mandatory if keyfile is not specified (hash can be 'plain' 
> which
> +means no hashing). Cipher must be specified with mode (for example, 
> 'aes-xts-plain64').
> +Key size must not exceed 128 bytes and must be specified in bits (for 
> example,
> +-s 256 or -s 512).
> +
> +Option @option{-S} determines encrypted disk sector size. It should be at 
> least
> +512 bytes and a power of 2. Note: current implementation of cryptsetup allows
> +only 512/ 1024/2048/4096 sectors. Disk sector size in plain mode is set at 
> encryption
> +time. Attempt to decrypt already created device with diffferent sector size 
> will
> +result in error.

I would prefer the last two sentences above to instead be:

  Disk sector size is configured when creating the encrypted volume.
  Attempting to decrypt a volume 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.

> +
> +Offset of encrypted data (device argument) and offset of keyfile (option 
> @option{-d})
> +can be specified with grub blocklist syntax ('+10M') where single word 
> suffixes 'K',
> +'M' and 'G' (base 1024) are available. Note: unlike other grub commands, 
> plainmount
> +supports only single blocklist offset.

Hmm, I wasn't suggesting this be added. I hope you didn't think I was
suggesting this. What I was suggesting was that the block list syntax
already supported in GRUB for device paths be used, not creating a new
block list syntax just for this command. You shouldn't need to add any
new code for what I was suggesting.

For instance, if you know that your plain mount volume is on device
(hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
the key material is offset 35235 bytes into that file you would use:

  loopback cplain0 (hd0)2048+
  plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)

If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
then use:

  plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)

or

  plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)

Here the '+' is needed after (hd1) to turn it into a file because -d
should only take a file. It would be nice to have (hd1) be treated as
(hd1)+ when used as a file, but that would be a different patch.

The drawback to what I'm suggesting is that you can't do "-d
(hd1)16K+". This could be something interesting to add to GRUB
blocklist syntax, but as a separate patch. 

I believe there's also a confusion here on the usage of blocklist
syntax. Blocklist syntax is about specifying a range of blocks, not an
offset or specific block number. So for instance, "(hd1)+16" means
blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
latter is what you want.

> +
> +By default new cryptodisk node will have uuid 
> '109fea84-a6b7-34a8-4bd1-1c506305a4e1'
> +where last digits are incremented for each subsequently created node. Custom
> +uuid can be specified with option @option{-u}.
> +
> +The plainmount command can report internal cryptographic errors. If they 
> happen,

Its not clear to me what internal cryptographic errors are reported,
under what conditions.

> +check hash, key size and cipher options - they should match each other.

"they should match the parameters used to create the volume."

> +Successfully decrypted disks are named as (cryptoX) and have linear 
> numeration
> +with other decrypted devices opened by cryptodisk.
> +
> +Note, all encryption arguments (cipher, hash, key size, disk offset and disk
> +sector size) must match those which were used to setup encrypted device. If
> +any of them does not match the actual arguments used during the initial 
> encryption,
> +plainmount will return garbage data and GRUB will report unknown filesystem 
> for the
> +opened disk. Note, writing data to such a virtual device will result in data
> +loss if the underlying partition contained desired data. If the encrypted 
> disk hosts
> +some high level abstraction (like LVM2 or MDRAID) it will be created under
> +separate device name.
> +@end deffn
> +
> +
>  @node play
>  @subsection play
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 715994872..579acd6c7 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1189,6 +1189,11 @@ module = {
>    common = disk/luks.c;
>  };
> 
> +module = {
> +  name = plainmount;
> +  common = disk/plainmount.c;
> +};
> +

This should be moved to after the "cryptodisk" module.

>  module = {
>    name = luks2;
>    common = disk/luks2.c;
> diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
> new file mode 100644
> index 000000000..086e5cb50
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,517 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2003,2007,2010,2011,2019  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/>.
> + */
> +
> +
> +#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+");
> +char PLAINMOUNT_DEFAULT_UUID[] =       
> "109fea84-a6b7-34a8-4bd1-1c506305a4e0";

I'd recommend the last two digits of the default UUID be '0'. This way
we can have up to 256 plain mount devices before creating
non-sequential UUIDs. Right now its 16, which seems like a lot, but
also seems like it could be on the edge of what someone might actually
want to do.

> +#define BITS_PER_BYTE                  8
> +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
> +
> +enum PLAINMOUNT_OPTION
> +  {
> +    OPTION_HASH,
> +    OPTION_CIPHER,
> +    OPTION_KEY_SIZE,
> +    OPTION_SECTOR_SIZE,
> +    OPTION_PASSWORD,
> +    OPTION_KEYFILE,
> +    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/disk path"), 0, ARG_TYPE_STRING},
> +    {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +struct grub_plainmount_args
> +{
> +  grub_uint8_t *key_data;
> +  char *cipher, *mode, *hash, *keyfile, *uuid;
> +  grub_size_t offset, key_size, sector_size, keyfile_offset;
> +  grub_disk_t disk;
> +};
> +typedef struct grub_plainmount_args *grub_plainmount_args_t;

I thought we were going to get rid of this?

> +
> +
> +/* Cryptodisk setkey() function wrapper */
> +static grub_err_t
> +plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *data,
> +                   grub_size_t size)
> +{
> +  gcry_err_code_t code = grub_cryptodisk_setkey (dev, data, size);
> +  if (code != GPG_ERR_NO_ERROR)
> +    {
> +      grub_dprintf ("plainmount", "password crypto status is %d\n", code);
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                         N_("cannot set key. Check consistency of "
> +                            "keysize/hash/cipher options."));

I don't really like this because grub_cryptodisk_setkey() should set
the error and we just pass it along. grub_cryptodisk_setkey() should
return a grub_err_t and not a gcry_err_code_t. Since it doesn't do
that, this is reasonable.

> +    }
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Parse disk size suffix */
> +static grub_size_t plainmount_parse_suffix (char *arg)

This should be removed. And possibly used in a separate patch for
adding this feature to blocklists in general, but it has issue as is as
well.

> +{
> +  const char *p;
> +  grub_size_t val = grub_strtoull (arg, &p, 0);
> +
> +  /* Only single word prefix is allowed */
> +  if (p[1] != '\0' || (p[0] != '\0' && p[0] != 'K' && p[0] != 'k' &&

If p[0] == '\0', then p[1] is an out of bounds access.

> +      p[0] != 'M' && p[0] != 'm' && p[0] != 'G' && p[0] != 'g'))
> +    return -1;

Perhaps set grub_errno here with grub_error(), but still return -1.

> +
> +  switch (*p)
> +    {
> +      case 'K':
> +      case 'k':
> +        val = val * 1024;
> +        break;
> +      case 'M':
> +      case 'm':
> +        val = val * 1024*1024;
> +        break;
> +      case 'G':
> +      case 'g':
> +        val = val * 1024*1024*1024;
> +        break;
> +      case '\0':
> +        break;
> +      default:
> +        val = -1;
> +    }
> +  return val;
> +}
> +
> +/* Support blocklist syntax */
> +static grub_size_t plainmount_parse_blocklist (char *arg)

This function should be gotten rid of too.

> +{
> +  char *pos = grub_strchr (arg, '+');

Filenames can have the '+' character on many file systems, so this is
not reliable.

> +
> +  if (!pos)
> +    return 0;
> +
> +  /* Erase '+', arg now points to disk, pos+1 to offset */
> +  pos[0] = '\0';
> +
> +  /* 'hd0,gpt2+' equals 'hd0,gpt2' equals no offset */
> +  if (pos[1] == '\0')
> +    return 0;
> +
> +  /* Blocklist syntax here supports K/M/G suffix */
> +  return plainmount_parse_suffix (pos+1);

As noted above, you're matching a number after the '+', which isn't
really correct.

> +}
> +
> +/* Configure cryptodisk uuid */
> +static void plainmount_set_uuid (grub_cryptodisk_t dev, char *user_uuid)
> +{
> +  grub_size_t pos = 0;
> +  static const char * def_uuid = PLAINMOUNT_DEFAULT_UUID;
> +
> +  /* Size of user_uuid is checked in main func */
> +  if (user_uuid)
> +      grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> +
> +  /* Set default UUID. Last digits start from 1 and are incremented for
> +     each new plainmount device. */
> +  else
> +    {
> +      /* Set uuid to '     ...x' where x starts from 1. */
> +      grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%32lu", dev->id+1);

The format type specifier should be 'x' instead of 'u' if you want
counting not to skip some UUIDs after the 10th mounted crypto devices.

Also, the format string should be "%36lx", if you want to create a 128
bit UUID. As it is right now, you're only writing a UUID with 112 bits
because PLAINMOUNT_DEFAULT_UUID has dashes which contribute not bits to
the UUID.

> +      while (dev->uuid[pos++] == ' ');
> +      grub_memcpy (dev->uuid, def_uuid, pos-1);

I'd say get rid of the "def_uuid" variable and just use
PLAINMOUNT_DEFAULT_UUID here.

> +    }
> +  COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof 
> (PLAINMOUNT_DEFAULT_UUID));
> +}
> +
> +
> +/* Configure cryptodevice sector size (-S option) */

This comment should also note that offset is also configured. But
actually, I don't think we need to do any configuring based on offset
and should just take that code out below. If the user needs an offset,
put that into a loopback device as in the examples I gave above.

> +static grub_err_t
> +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t disk,
> +                              grub_size_t sector_size, grub_size_t offset)
> +{
> +  grub_disk_addr_t total_sectors;
> +  dev->log_sector_size = grub_log2ull(sector_size);

Nit, space before '('.

> +
> +  /* Convert size to sectors */
> +  total_sectors = grub_disk_native_sectors (disk);
> +  if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +                       N_("cannot determine disk %s size"), disk->name);
> +
> +  total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> +                                       dev->log_sector_size);
> +  if (total_sectors == 0)
> +    return grub_error (GRUB_ERR_BAD_DEVICE,
> +                       N_("cannot set specified sector size on disk %s"), 
> disk->name);
> +
> +  /* Convert offset to sectors */
> +  dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
> +  if (total_sectors <= dev->offset_sectors)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                       N_("specified disk offset is larger than disk size"));
> +
> +  /* Adjust device size for offset */
> +  dev->total_sectors = total_sectors - dev->offset_sectors;
> +  grub_dprintf ("plainmount", "log_sector_size=%d, 
> total_sectors=%"PRIuGRUB_SIZE
> +                ", offset_sectors=%"PRIuGRUB_SIZE"\n", dev->log_sector_size,
> +                dev->total_sectors, dev->offset_sectors);
> +  return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Hashes a password into a key and stores it with cipher. */
> +static grub_err_t
> +plainmount_configure_password (grub_cryptodisk_t dev, grub_disk_t disk, char 
> *hash,
> +                               grub_uint8_t *key_data, grub_size_t key_size)
> +{
> +  const gcry_md_spec_t *gcry_hash;
> +  grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = 
> derived_hash;
> +  char *p;
> +  unsigned int round, i;
> +  unsigned int len, size;
> +
> +  /* Option -p was not set */
> +  if (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"));
> +  }
> +
> +  /* Support none (plain) hash */
> +  if (grub_strcmp (hash, "plain") == 0)
> +    {
> +      dev->hash = NULL;
> +      return plainmount_setkey (dev, key_data, key_size);
> +    }
> +
> +  /* Check hash */
> +  gcry_hash = grub_crypto_lookup_md_by_name (hash);
> +  if (!gcry_hash)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> +                       N_("couldn't load %s hash"),
> +                       hash);
> +
> +  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 * BITS_PER_BYTE,
> +                           GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
> +
> +  dev->hash = gcry_hash;
> +  len = dev->hash->mdlen;
> +  p = grub_malloc (key_size + 2 + key_size / len);

I think its a little better to have parentheses around "key_size / len".

> +  if (!p)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +
> +  /* Hash password. Adapted from cryptsetup.
> +     https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> +  */

Multi-line comments in GRUB are supposed to be like:

  /*
   * 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);
> +
> +      if (len > size)
> +     len = size;
> +
> +      grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> +    }
> +  grub_free (p);
> +  return plainmount_setkey (dev, derived_hash, key_size);
> +}
> +
> +
> +/* Read keyfile as a file */
> +static grub_err_t
> +plainmount_configure_keyfile (grub_cryptodisk_t dev, 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 (!keyfile)
> +      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 plainmount_setkey (dev, key_data, key_size);
> +}
> +
> +
> +/* Read keyfile as a disk segment */
> +static grub_err_t
> +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, 
> grub_uint8_t *key_data,
> +                              grub_size_t key_size, grub_size_t 
> keyfile_offset)

I don't think this function should exist either. Using GRUB's already
existing blocklist syntax (see example above) and with -O for
specifying keyfile offset, we don't need this.

> +{
> +  char *keydisk_name = grub_file_get_device_name (keyfile);
> +  grub_disk_t keydisk = grub_disk_open (keyfile);
> +  if (!keydisk)
> +    {
> +      grub_free (keydisk_name);
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unable to open key disk 
> %s"), keydisk_name);
> +    }
> +  if (grub_disk_read (keydisk, 0, keyfile_offset, key_size, key_data) != 
> GRUB_ERR_NONE)
> +    {
> +      grub_free (keydisk_name);
> +      grub_disk_close (keydisk);
> +      return grub_error (GRUB_ERR_READ_ERROR, N_("failed to read key from 
> disk %s"), keydisk_name);
> +    }
> +  return plainmount_setkey (dev, key_data, key_size);
> +}
> +
> +
> +/* 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;
> +  struct grub_plainmount_args cargs = {0};

I thought were were going to gt rid of this? Now cargs is localized to
only this function, so its even less needed.

> +  grub_cryptodisk_t dev = NULL;
> +  char *diskname, *disklast = NULL;
> +  grub_size_t len;
> +  grub_err_t err;
> +  const char *p;
> +
> +  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 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) + 1 > 
> 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_("device UUID exceeds 
> maximum size"));
> +
> +  /* Parse cmdline arguments */
> +  cargs.offset = plainmount_parse_blocklist (args[0]);
> +  if (cargs.offset == (grub_size_t)-1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse disk %s"), 
> args[0]);

Let's not create new blocklist syntax parsing/handling and instead let
the user use GRUB's already built in syntax while creating a loopback
device. This should allow us to not have to worry about offset at all.
As noted above, if its really desirable to specify encrypted blocks like
"(hd1)16M+", then that should be a separate patch to the already
existing blocklist parser. I'm not opposed to that either, I think it
could be convenient syntax. It just shouldn't be in this patch.

> +  if (state[OPTION_KEYFILE].set)
> +    {
> +      cargs.keyfile_offset = plainmount_parse_blocklist 
> (state[OPTION_KEYFILE].arg);
> +      if (cargs.keyfile_offset == (grub_size_t)-1)
> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse keyfile 
> %s"),
> +                           state[OPTION_KEYFILE].arg);

And here, let's go back to setting keyfile_offset based on the -O
parameter. This has the added benefit of being inline with cryptomount
options.

> +    }
> +
> +  grub_errno = GRUB_ERR_NONE;
> +  cargs.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 && (*p != '\0' || grub_errno != 
> GRUB_ERR_NONE))

The concensus now seems to be that the proper way to handle errors in
grub_strtoull() is the following:

  if (state[OPTION_SECTOR_SIZE].set && (state[OPTION_SECTOR_SIZE].arg
        == '\0' || *p != '\0'))

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector 
> size"));
> +
> +  cargs.key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0) / 
> BITS_PER_BYTE;
> +  if (*p != '\0' || grub_errno != GRUB_ERR_NONE)

Similar to above comment.

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
> +
> +  /* Check key size */
> +  if (cargs.key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("key size %"PRIuGRUB_SIZE
> +                                                 " exceeds maximum %d bits"),
> +                       cargs.key_size * BITS_PER_BYTE,
> +                       GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
> +
> +  /* Check disk sector size */
> +  if (cargs.sector_size < 512)

s/512/GRUB_DISK_SECTOR_SIZE/

> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "sector size -S must be at 
> least 512");

Parameterize 512 in this string and use GRUB_DISK_SECTOR_SIZE.

> +  if ((cargs.sector_size & (cargs.sector_size - 1)) != 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                       N_("sector size -S %"PRIuGRUB_SIZE" is not power of 
> 2"),
> +                       cargs.sector_size);
> +
> +  /* Allocate all stuff here */
> +  cargs.hash =  state[OPTION_HASH].set ? grub_strdup 
> (state[OPTION_HASH].arg) : NULL;
> +  cargs.cipher = grub_strdup (state[OPTION_CIPHER].arg);
> +  cargs.keyfile = state[OPTION_KEYFILE].set ? grub_strdup 
> (state[OPTION_KEYFILE].arg) : NULL;
> +  dev = grub_zalloc (sizeof *dev);
> +  cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +  cargs.uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) 
> : NULL;
> +  if ((!cargs.hash && state[OPTION_HASH].set) || !cargs.cipher ||
> +      (!cargs.keyfile && state[OPTION_KEYFILE].set) || !dev || 
> !cargs.key_data ||
> +      (!cargs.uuid && 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 (cargs.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 (cargs.uuid, state[OPTION_UUID].arg, grub_strlen 
> (state[OPTION_UUID].arg));
> +
> +  /* Set cipher mode (tested above) */
> +  cargs.mode = grub_strchr (cargs.cipher,'-');
> +  *cargs.mode++ = '\0';
> +
> +  /* Check cipher */
> +  if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != 
> GRUB_ERR_NONE)
> +    {
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                     N_("invalid cipher %s"), cargs.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++;
> +    }
> +  cargs.disk = grub_disk_open (diskname);
> +  if (!cargs.disk)
> +    {
> +      if (disklast)
> +        *disklast = ')';
> +      err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                        N_("cannot open disk %s"), diskname);
> +      goto exit;
> +    }
> +
> +  /* Warn if hash and keyfile are both provided */
> +  if (cargs.keyfile && 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"));
> +
> +  grub_dprintf ("plainmount",
> +              "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE
> +           ", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> +              cargs.cipher, cargs.hash, cargs.key_size,
> +              cargs.keyfile, cargs.keyfile_offset);
> +
> +  err = plainmount_configure_sectors (dev, cargs.disk, cargs.sector_size, 
> cargs.offset);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  /* Configure keyfile/keydisk/password */
> +  if (cargs.keyfile)
> +    if (cargs.keyfile[0] == '/' ||
> +       (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> +      err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> +                                          cargs.key_size, 
> cargs.keyfile_offset);
> +    else
> +      err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> +                                          cargs.key_size, 
> cargs.keyfile_offset);

We shouldn't support sending a device as a keyfile and only support
files. As noted above, if the keyfile data is only accessibly via some
blocks on a disk device, then use the builtin blocklist syntax
potentially with the -O keyfile offset.

> +  else
> +    err = plainmount_configure_password (dev, cargs.disk, cargs.hash,
> +                                         cargs.key_data, cargs.key_size);
> +  if (err != GRUB_ERR_NONE)
> +    goto exit;
> +
> +  err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
> +  if (err != GRUB_ERR_NONE)
> +    {
> +      grub_printf_ (N_("cannot initialize cryptodisk. Check consistency "
> +                       "of cipher/key size/hash arguments.\n"));

This error message will only get printed when grub_cryptodisk_insert()
fails, which only happens when the grub_strdup() that is calls fails.
Thus this error message does not fit the error condition. This error
has nothing to do with the correctness of "cipher/key size/hash
arguments", but is instead an out of memory error.

> +      goto exit;
> +    }
> +
> +  dev->modname = "plainmount";
> +  dev->source_disk = cargs.disk;
> +  plainmount_set_uuid (dev, cargs.uuid);
> +
> +exit:
> +  grub_free (cargs.hash);
> +  grub_free (cargs.cipher);
> +  grub_free (cargs.keyfile);
> +  grub_free (cargs.key_data);
> +  grub_free (cargs.uuid);
> +  if (err != GRUB_ERR_NONE && cargs.disk)
> +    grub_disk_close (cargs.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] [-p password] [-u uuid] "
> +                           " [-d keyfile] <SOURCE>"),
> +                           N_("Open partition encrypted in plain mode."),
> +                           options);
> +}
> +
> +GRUB_MOD_FINI (plainmount)
> +{
> +  grub_unregister_extcmd (cmd);
> +}
> --
> 2.36.1
> 

Glenn