[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 1/1] plainmount: Support plain encryption mode
|
From: |
Glenn Washburn |
|
Subject: |
Re: [PATCH v4 1/1] plainmount: Support plain encryption mode |
|
Date: |
Sun, 10 Jul 2022 16:07:53 -0500 |
On Sat, 02 Jul 2022 17:44:37 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> From 109488f1aa001682b7184ae830a785ee13b92cce Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sat, 2 Jul 2022 18:32:48 +0100
> Subject: [PATCH v4 1/1] plainmount: Support plain encryption mode
>
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
> ---
> docs/grub.texi | 78 +++++++
> grub-core/Makefile.core.def | 5 +
> grub-core/disk/plainmount.c | 408 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 491 insertions(+)
> create mode 100644 grub-core/disk/plainmount.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 9b902273c..27887b083 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.
s/plainmount/@command{plainmount}/
> @end deffn
>
> @node cutmem
> @@ -5017,6 +5021,80 @@ 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{-o} offset] [@option{-p} password]
> [@option{-d} keyfile] [@option{-O} keyfile offset]
> +[@option{-u} uuid]
> +
> +Setup access to the encrypted device in plain mode. The device argument can
> point
> +to a disk, partition or to a loopback file. Offset of the encrypted data at
> the
> +device can be specified in terms of 512 byte sectors with the blocklist
> syntax and
> +loopback file or in terms of arbitrary number of bytes with the option
> @option{o}.
> +Example:
> +
> +
> +loopback node (hd0,gpt1)2048+
> +
> +plainmount node
Let's wrap this in @example, like:
@example
loopback node (hd0,gpt1)2048+
plainmount node
@end example
> +
> +plainmount hd0,gpt1 -o 1048576
> +
> +
> +both create virtual devices with 1MiB offset on top of the specified
> partition. The
> +option @option{-o} is useful to specify offset which is not aligned to 512
> byte
I just noticed that the code takes this parameter and turns it into a
number of sectors that the data is offset from the start of the device.
Thus internally this gets rounded down to be sector aligned anyway. In
fact, its aligned to the size of the sector specified with the -S
option. So we should get rid of the -o entirely. In fact, -o will not
work if it is not a multiple of the sector size specified with -S.
> +sector size. Note: current cryptsetup implementation of plain mode and LUKS
> v1 restricts
> +alignment to 512 byte sector size. Specifying arbitrary byte alignment is
> useful only to
> +open LUKS v2 volume if master key is known or to open the volume encrypted
> by other
> +cryptographic implementation. Note: in LUKS revealing master key is not
> recommended
> +because it allows to open encrypted device directly bypassing the header and
> LUKS
> +security features.
These "Notes" should probably be reorganized as @footnote{Put note
inside of this.} You have two notes here, so perhaps two foot notes,
although it looks like here you have a nested note. I'm not sure if you
can have nested footnotes.
> +
> +Password can be specified in two ways: as a password data from a keyfile or
> as a secret
> +passphrase. The keyfile parameter @option{-d} specifies path to the keyfile
> containing
> +password data and has higher priority than the other password method.
> Specified password
> +data in this mode is not hashed. The option @option{-O} specifies offset in
> terms of bytes
Hmm its not hashed? I thought a keyfile that contains 4 characters
"pass" would be the same as entering "pass" as the secret passphrase.
If the keyfile is not hashed, then the derived master key is different.
Are you Am I mistaken or missing something?
> +of the password data from the start of the keyfile. This option has no
> effect without the
> +option @option{-d}. Offset of password data in the keyfile can be specified
> directly with
> +option @option{-d} and GRUB blocklist syntax (for example: '-d
> (hd0,gpt1)2048+').
Should this be -O instead of -d?
> +
> +If no keyfile specified then the password supplied with the option
> @option{-p} is used or
> +the password is requested interactively from the console. In both cases
> 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 'plain' which
> means that no
> +hashing is done.
> +
> +Cipher @option{-c} and keysize @option{-s} options specify the cipher
> algorithm and the
> +key size respectively and are mandatory options (apply to all three methods
> to specify the
> +password). Cipher must be specified with the 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).
> +
> +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. Note: current
> implementation
> +of cryptsetup supports only 512/1024/2048/4096 byte sectors. Disk sector
> size is
Use @footnote here too.
> +configured when creating the encrypted volume. Attempting to decrypt such
> volume with
> +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.
Actually, some sectors will be decrypted correctly. So perhaps we
should say "but not all sectors will be properly decrypted, generally
causing the volume to be inaccessible"
> +
> +By default new cryptodisk node will have uuid
> '109fea84-a6b7-34a8-4bd1-1c506305a401'
> +where last digits are incremented for each subsequently created node. Custom
> +uuid can be specified with the option @option{-u}.
Perhaps a footnote here saying that default created UUIDs will be
sequential up to X number of devices. It looks like 2^10 devices.
> +
> +Successfully decrypted disks are named as (cryptoX) and have linear
> numeration
> +with other decrypted by cryptodisk devices. If the encrypted disk hosts some
> higher
> +level abstraction (like LVM2 or MDRAID) it will be created under a separate
> device
> +namespace in addition to the cryptodisk namespace.
This is standard cryptomount behavior. I think it makes more sense to
have some version of this paragraph in cryptomount and make a note here
that it behaves like cryptomount.
> +
> +Note, 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. Note, writing data to such virtual device will result in the data
> loss if the
I'd say remove "Note, " here and starting with "Writing".
> +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..ca92f1ee6
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,408 @@
> +/*
> + * 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-1c506305a400";
> +#define BITS_PER_BYTE 8
> +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
Minor nit, I'd rather have these defines before the definition of
PLAINMOUNT_DEFAULT_UUID.
> +
> +enum PLAINMOUNT_OPTION
> + {
> + OPTION_HASH,
> + OPTION_CIPHER,
> + OPTION_KEY_SIZE,
> + OPTION_SECTOR_SIZE,
> + OPTION_OFFSET,
> + 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},
> + {"offset", 'o', 0, N_("Device offset"), 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 *data,
> + grub_size_t size)
Perhaps better to use the name "key" instead of "data", so that its
very clear what "data" should be.
> +{
> + 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 specified
> key"));
> + }
> + return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Configure cryptodisk uuid */
> +static void plainmount_set_uuid (grub_cryptodisk_t dev, char *user_uuid)
Use const char.
> +{
> + grub_size_t pos = 0;
> +
> + /* Size of user_uuid is checked in main func */
> + if (user_uuid)
> + 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.
> + * snprintf() sets uuid to ' ...x' where x starts from 1.
> + */
> + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
> + while (dev->uuid[pos++] == ' ');
> + grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos-1);
I'm wondering if there's a good reason to have PLAINMOUNT_DEFAULT_UUID
not be a #define, which I think makes more sense and would be expected
if just reading this part of the code.
> + }
> + 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 offset,
It looks like this line is over 80 chars. Perhaps move the third
parameter to the next line.
> + 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);
Nit, this line seems not to be indented properly.
> + if (dev->total_sectors == 0)
> + return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot set specified sector
> size on disk %s"),
> + disk->name);
> +
> + /* Configure device offset */
> + dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
Something I didn't catch before. Since there is only offset_sectors,
there can be no non-sector aligned offsets. Thus it really doesn't make
sense to have a '-o' option, blocklists will do everything that is
needed, and more actually. If you have a plainmount volume with sector
size 4096 at offset 2048 in (hd0), you can not access it using "-o
2048". That will cause grub_divmod64() above to return 0 (2048/4096).
However creating a loopback device using "(hd0)+4" will work. This is
partly a limitation of the current cryptodisk code. Perhaps there
should be a dev->offset_partial that is the number of bytes to add to
the offset_sectors to get the byte offset. That would be a different
patch series though. And I'm not convinced that its very useful (if you
have a convincing use-case I'm open to it).
> + if (dev->total_sectors <= dev->offset_sectors)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("specified disk offset is
> larger than disk size"));
> + dev->total_sectors = dev->total_sectors - dev->offset_sectors;
> +
> + 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_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);
Have this parameter on a separate line and properly indented to prevent
exceeding 80 chars on this line.
> +
> + dev->hash = gcry_hash;
> + len = dev->hash->mdlen;
> + p = grub_malloc (key_size + 2 + (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
> + */
> + 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);
Space after (char*).
> +
> + 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 key material from keyfile */
> +static grub_err_t
> +plainmount_configure_keyfile (grub_cryptodisk_t dev, char *keyfile,
> grub_uint8_t *key_data,
I don't think key_data needs to be passed here. Its only used to put
data read from the key file, but never used outside of this function.
My guess is that you're wanting to avoid allocating a buffer of
GRUB_CRYPTODISK_MAX_PASSPHRASE twice. But its strange to have a
function that takes a buffer that it unconditionally overwrites and
where that buffer is not used after the function.
Thinking about it more, plainmount_configure_password and
plainmount_configure_keyfile should probably be refactored. I'm
thinking that neither function should call plainmount_setkey(), but
should have key_data as an out parameter and have plainmount_setkey()
called from grub_cmd_plainmount(). I'm also open to something other
than my above suggestion too.
> + 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)
> + 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);
> +}
> +
> +
> +/* 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;
> + char *diskname, *disklast = NULL, *cipher, *mode, *hash, *keyfile, *uuid;
> + grub_size_t len, key_size, offset, sector_size, keyfile_offset = 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 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_("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"));
> +
> + key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0) /
> BITS_PER_BYTE;
I'm thinking we should not do the division here, and after the check
below, check that the key_size is a multiple of BITS_PER_BYTE and if
not, return error. Right now, if I use "-s 259", key_size will be 32,
which is the same as if I used "-s 256". But 259 really is an invalid
key_size.
> + 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"));
> +
> + offset = state[OPTION_OFFSET].set ? grub_strtoull
> (state[OPTION_OFFSET].arg, &p, 0) : 0;
> + if (state[OPTION_OFFSET].arg[0] == '\0' || *p != '\0' || grub_errno !=
> GRUB_ERR_NONE)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized offset"));
> +
> + /* Check key size */
> + 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 * BITS_PER_BYTE, GRUB_CRYPTODISK_MAX_KEYLEN
> * BITS_PER_BYTE);
> +
> + /* 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 && state[OPTION_HASH].set) || !cipher ||
> + (!keyfile && state[OPTION_KEYFILE].set) || !dev || !key_data ||
> + (!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 (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)
> + {
> + 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 (keyfile && state[OPTION_HASH].arg)
Daniel's preference is to do "keyfile == NULL" and similarly for all
pointer checks.
> + 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, offset, sector_size);
> + if (err != GRUB_ERR_NONE)
> + goto exit;
> +
> + /* Configure keyfile or password */
> + if (keyfile)
Ditto.
> + err = plainmount_configure_keyfile (dev, keyfile, key_data, key_size,
> keyfile_offset);
> + else
> + err = plainmount_configure_password (dev, disk, hash, key_data,
> key_size);
> + if (err != GRUB_ERR_NONE)
> + goto exit;
> +
My suggestion would be to have plainmount_setkey() here.
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.0
>
>