* [PATCH v4 1/1] plainmount: Support plain encryption mode
@ 2022-07-02 17:44 Maxim Fomin
2022-07-10 21:07 ` Glenn Washburn
0 siblings, 1 reply; 4+ messages in thread
From: Maxim Fomin @ 2022-07-02 17:44 UTC (permalink / raw)
To: grub-devel; +Cc: development, ps
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.
@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
+
+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
+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.
+
+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
+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+').
+
+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
+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.
+
+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}.
+
+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.
+
+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
+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
+
+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)
+{
+ 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)
+{
+ 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);
+ }
+ 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,
+ 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);
+
+ /* Configure device offset */
+ dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
+ 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);
+
+ 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);
+
+ 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,
+ 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;
+ 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)
+ 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)
+ 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;
+
+ 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v4 1/1] plainmount: Support plain encryption mode
2022-07-02 17:44 [PATCH v4 1/1] plainmount: Support plain encryption mode Maxim Fomin
@ 2022-07-10 21:07 ` Glenn Washburn
2022-07-11 19:35 ` Maxim Fomin
0 siblings, 1 reply; 4+ messages in thread
From: Glenn Washburn @ 2022-07-10 21:07 UTC (permalink / raw)
To: Maxim Fomin; +Cc: grub-devel, ps
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v4 1/1] plainmount: Support plain encryption mode
2022-07-10 21:07 ` Glenn Washburn
@ 2022-07-11 19:35 ` Maxim Fomin
2022-07-14 21:02 ` Glenn Washburn
0 siblings, 1 reply; 4+ messages in thread
From: Maxim Fomin @ 2022-07-11 19:35 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Sunday, July 10th, 2022 at 9:07 PM, Glenn Washburn <development@efficientek.com> wrote:
> > +
> > +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.
Option -o is taken as a byte number, not as a sector number, it is not multiplied
by 512 or by sector size. However later, indeed it is truncated to a sector size,
loosing its meaning. I agree with the rest of the comment, this option is irrelevant
and should be removed.
>
> > +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.
I can change them to be just a single note.
> > +
> > +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?
I have taken this behavior from cryptsetup. Having binary key file data directly allows
to have arbitrary password (not necessarily typable with a keyboard). Such feature is not
very popular, but it exists. This feature is connected with use cases where user types
password in non-English keyboard (possibly, a letter which is not easily encoded in UTF,
or which encoding depends on keyboard layout), which cannot be typed at another computer.
> > +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?
Why? The last part of quoted text says that alternative to option -O is the blocklist syntax
which is part of the -d option.
> > +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"
I would not complicate explanation. The difference between decrypted in such 'strange'
scenario FS and normal FS is so large, that no fs will work. For example, I have a BTRFS
on top of -S 4096 encrypted partition. Once on live system (not GRUB) I forgot to type
-S 4096 and received internal BTRFS error in dmesg saying something about bad internal
metadata state and then general mount error. So yes, BTRFS can recognize that this is a
BTRFS partition, but will refuse to mount. In GRUB mode such device (wrong sector size)
is not recognized too. I think what can be added here is at most a line saying that FS
may ocassionally detect underlying FS, but will refuse to mount.
> > +
> > +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.
Do you mean to add this information in cryptomount doc as a part of this patch?
> > +#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.
The comment later says about making it a #define instead of char[]. I made it
char[] because it was more convenient for previous version of the code which
is not true in current version. Yes, I can change that.
>
> > +{
> > + 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.
OK.
> > + 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).
I also think that -o option is useless in current conditions.
> > +
> > + 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.
I am OK with moving a call to setkey() outside of these two functions.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4 1/1] plainmount: Support plain encryption mode
2022-07-11 19:35 ` Maxim Fomin
@ 2022-07-14 21:02 ` Glenn Washburn
0 siblings, 0 replies; 4+ messages in thread
From: Glenn Washburn @ 2022-07-14 21:02 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Mon, 11 Jul 2022 19:35:47 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
> On Sunday, July 10th, 2022 at 9:07 PM, Glenn Washburn <development@efficientek.com> wrote:
>
> > > +
> > > +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.
>
> Option -o is taken as a byte number, not as a sector number, it is not multiplied
> by 512 or by sector size. However later, indeed it is truncated to a sector size,
> loosing its meaning. I agree with the rest of the comment, this option is irrelevant
> and should be removed.
I never did say above that the -o value gets multiplied, it gets
aligned. Anyway, I think we agree here.
>
> >
> > > +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.
>
> I can change them to be just a single note.
>
> > > +
> > > +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?
>
> I have taken this behavior from cryptsetup. Having binary key file data directly allows
> to have arbitrary password (not necessarily typable with a keyboard). Such feature is not
> very popular, but it exists. This feature is connected with use cases where user types
> password in non-English keyboard (possibly, a letter which is not easily encoded in UTF,
> or which encoding depends on keyboard layout), which cannot be typed at another computer.
Ok, yes, I was mistaken. Perhaps we should say after "not hashed", ",
and and will be used as the cipher key."
>
> > > +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?
>
> Why? The last part of quoted text says that alternative to option -O is the blocklist syntax
> which is part of the -d option.
Ok, I got confused by the wording. I think it should use "using" instead
of "and", eg. "Offset of password data in the keyfile can be specified
directly with option @option{-d} using GRUB blocklist syntax"
>
> > > +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"
>
> I would not complicate explanation. The difference between decrypted in such 'strange'
> scenario FS and normal FS is so large, that no fs will work. For example, I have a BTRFS
> on top of -S 4096 encrypted partition. Once on live system (not GRUB) I forgot to type
> -S 4096 and received internal BTRFS error in dmesg saying something about bad internal
> metadata state and then general mount error. So yes, BTRFS can recognize that this is a
> BTRFS partition, but will refuse to mount. In GRUB mode such device (wrong sector size)
> is not recognized too. I think what can be added here is at most a line saying that FS
> may ocassionally detect underlying FS, but will refuse to mount.
Your suggestion works for me.
>
> > > +
> > > +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.
>
> Do you mean to add this information in cryptomount doc as a part of this patch?
I hadn't thought much about it, but I think its fine to add to this
patch.
>
> > > +#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.
>
> The comment later says about making it a #define instead of char[]. I made it
> char[] because it was more convenient for previous version of the code which
> is not true in current version. Yes, I can change that.
>
>
> >
> > > +{
> > > + 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.
>
> OK.
>
> > > + 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).
>
> I also think that -o option is useless in current conditions.
>
> > > +
> > > + 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.
>
> I am OK with moving a call to setkey() outside of these two functions.
Glenn
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-14 21:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-02 17:44 [PATCH v4 1/1] plainmount: Support plain encryption mode Maxim Fomin
2022-07-10 21:07 ` Glenn Washburn
2022-07-11 19:35 ` Maxim Fomin
2022-07-14 21:02 ` Glenn Washburn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.