* [PATCH v3 1/1] plainmount: Support plain encryption mode.
@ 2022-06-18 14:33 Maxim Fomin
2022-06-25 0:55 ` Glenn Washburn
0 siblings, 1 reply; 9+ messages in thread
From: Maxim Fomin @ 2022-06-18 14:33 UTC (permalink / raw)
To: grub-devel; +Cc: development, ps
From 87af7e9cbeb72c3bc146564f64aa8132c1bf6d68 Mon Sep 17 00:00:00 2001
From: Maxim Fomin <maxim@fomin.one>
Date: Sat, 18 Jun 2022 14:32:49 +0100
Subject: [PATCH v3 1/1] plainmount: Support plain encryption mode.
This patch adds support for plain encryption mode (plain dm-crypt) via
new module/command named 'plainmount'.
Changes from the second version of the patch:
- added support for blocklist syntax offset
- added explicit error for unsupported offset syntax
- removed offset options -o and -O
- removed disk size -b option
- added static compile-time uuid
- removed 4096 sector size limitation
- parameters are passed directly (not inside struct)
- added support for plain hash
- minor code improvements
- improved documentation
---
docs/grub.texi | 54 ++++
grub-core/Makefile.core.def | 5 +
grub-core/disk/plainmount.c | 517 ++++++++++++++++++++++++++++++++++++
3 files changed, 576 insertions(+)
create mode 100644 grub-core/disk/plainmount.c
diff --git a/docs/grub.texi b/docs/grub.texi
index 9b902273c..a72475a8b 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4229,6 +4229,7 @@ you forget a command, you can run the command @command{help}
* parttool:: Modify partition table entries
* password:: Set a clear-text password
* password_pbkdf2:: Set a hashed password
+* plainmount:: Open device encrypted in plain mode.
* play:: Play a tune
* probe:: Retrieve device info
* rdmsr:: Read values from model-specific registers
@@ -4509,6 +4510,9 @@ function is supported, as Argon2 is not yet supported.
Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
specified without dash separators.
+
+Support for plain encryption mode (plain dm-crypt) is provided via separate
+plainmount command.
@end deffn
@node cutmem
@@ -5017,6 +5021,56 @@ to generate password hashes. @xref{Security}.
@end deffn
+@node plainmount
+@subsection plainmount
+
+@deffn Command plainmount device @option{-c} cipher @option{-s} key size [@option{-h} hash]
+[@option{-S} sector size] [@option{-p} password] [@option{-d} keyfile] [@option{-u} uuid]
+
+Setup access to encrypted in plain mode device. Password can be specified in
+two ways: as a secret passphrase or as a byte array from a keyfile. Secret
+passphrase can be provided interactively from console or with option @option{-p}.
+The keyfile is specified with option @option{-d} and can be either a path to a
+regular file or a block device. The keyfile parameter @option{-d} has higher
+priority than option @option{-p}.
+
+Cipher @option{-c} and keysize @option{-s} options are mandatory. Hash option
+@option{-h} is mandatory if keyfile is not specified (hash can be 'plain' which
+means no hashing). Cipher must be specified with mode (for example, 'aes-xts-plain64').
+Key size must not exceed 128 bytes and must be specified in bits (for example,
+-s 256 or -s 512).
+
+Option @option{-S} determines encrypted disk sector size. It should be at least
+512 bytes and a power of 2. Note: current implementation of cryptsetup allows
+only 512/ 1024/2048/4096 sectors. Disk sector size in plain mode is set at encryption
+time. Attempt to decrypt already created device with diffferent sector size will
+result in error.
+
+Offset of encrypted data (device argument) and offset of keyfile (option @option{-d})
+can be specified with grub blocklist syntax ('+10M') where single word suffixes 'K',
+'M' and 'G' (base 1024) are available. Note: unlike other grub commands, plainmount
+supports only single blocklist offset.
+
+By default new cryptodisk node will have uuid '109fea84-a6b7-34a8-4bd1-1c506305a4e1'
+where last digits are incremented for each subsequently created node. Custom
+uuid can be specified with option @option{-u}.
+
+The plainmount command can report internal cryptographic errors. If they happen,
+check hash, key size and cipher options - they should match each other.
+Successfully decrypted disks are named as (cryptoX) and have linear numeration
+with other decrypted devices opened by cryptodisk.
+
+Note, all encryption arguments (cipher, hash, key size, disk offset and disk
+sector size) must match those which were used to setup encrypted device. If
+any of them does not match the actual arguments used during the initial encryption,
+plainmount will return garbage data and GRUB will report unknown filesystem for the
+opened disk. Note, writing data to such a virtual device will result in data
+loss if the underlying partition contained desired data. If the encrypted disk hosts
+some high level abstraction (like LVM2 or MDRAID) it will be created under
+separate device name.
+@end deffn
+
+
@node play
@subsection play
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 715994872..579acd6c7 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1189,6 +1189,11 @@ module = {
common = disk/luks.c;
};
+module = {
+ name = plainmount;
+ common = disk/plainmount.c;
+};
+
module = {
name = luks2;
common = disk/luks2.c;
diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
new file mode 100644
index 000000000..086e5cb50
--- /dev/null
+++ b/grub-core/disk/plainmount.c
@@ -0,0 +1,517 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2003,2007,2010,2011,2019 Free Software Foundation, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+#include <grub/cryptodisk.h>
+#include <grub/dl.h>
+#include <grub/err.h>
+#include <grub/extcmd.h>
+#include <grub/partition.h>
+#include <grub/file.h>
+
+
+GRUB_MOD_LICENSE ("GPLv3+");
+char PLAINMOUNT_DEFAULT_UUID[] = "109fea84-a6b7-34a8-4bd1-1c506305a4e0";
+#define BITS_PER_BYTE 8
+#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
+
+enum PLAINMOUNT_OPTION
+ {
+ OPTION_HASH,
+ OPTION_CIPHER,
+ OPTION_KEY_SIZE,
+ OPTION_SECTOR_SIZE,
+ OPTION_PASSWORD,
+ OPTION_KEYFILE,
+ OPTION_UUID
+ };
+
+static const struct grub_arg_option options[] =
+ {
+ /* TRANSLATORS: It's still restricted to this module only. */
+ {"hash", 'h', 0, N_("Password hash"), 0, ARG_TYPE_STRING},
+ {"cipher", 'c', 0, N_("Password cipher"), 0, ARG_TYPE_STRING},
+ {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
+ {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
+ {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
+ {"keyfile", 'd', 0, N_("Keyfile/disk path"), 0, ARG_TYPE_STRING},
+ {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
+ {0, 0, 0, 0, 0, 0}
+ };
+
+struct grub_plainmount_args
+{
+ grub_uint8_t *key_data;
+ char *cipher, *mode, *hash, *keyfile, *uuid;
+ grub_size_t offset, key_size, sector_size, keyfile_offset;
+ grub_disk_t disk;
+};
+typedef struct grub_plainmount_args *grub_plainmount_args_t;
+
+
+/* Cryptodisk setkey() function wrapper */
+static grub_err_t
+plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *data,
+ grub_size_t size)
+{
+ gcry_err_code_t code = grub_cryptodisk_setkey (dev, data, size);
+ if (code != GPG_ERR_NO_ERROR)
+ {
+ grub_dprintf ("plainmount", "password crypto status is %d\n", code);
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot set key. Check consistency of "
+ "keysize/hash/cipher options."));
+ }
+ return GRUB_ERR_NONE;
+}
+
+
+/* Parse disk size suffix */
+static grub_size_t plainmount_parse_suffix (char *arg)
+{
+ const char *p;
+ grub_size_t val = grub_strtoull (arg, &p, 0);
+
+ /* Only single word prefix is allowed */
+ if (p[1] != '\0' || (p[0] != '\0' && p[0] != 'K' && p[0] != 'k' &&
+ p[0] != 'M' && p[0] != 'm' && p[0] != 'G' && p[0] != 'g'))
+ return -1;
+
+ switch (*p)
+ {
+ case 'K':
+ case 'k':
+ val = val * 1024;
+ break;
+ case 'M':
+ case 'm':
+ val = val * 1024*1024;
+ break;
+ case 'G':
+ case 'g':
+ val = val * 1024*1024*1024;
+ break;
+ case '\0':
+ break;
+ default:
+ val = -1;
+ }
+ return val;
+}
+
+/* Support blocklist syntax */
+static grub_size_t plainmount_parse_blocklist (char *arg)
+{
+ char *pos = grub_strchr (arg, '+');
+
+ if (!pos)
+ return 0;
+
+ /* Erase '+', arg now points to disk, pos+1 to offset */
+ pos[0] = '\0';
+
+ /* 'hd0,gpt2+' equals 'hd0,gpt2' equals no offset */
+ if (pos[1] == '\0')
+ return 0;
+
+ /* Blocklist syntax here supports K/M/G suffix */
+ return plainmount_parse_suffix (pos+1);
+}
+
+/* Configure cryptodisk uuid */
+static void plainmount_set_uuid (grub_cryptodisk_t dev, char *user_uuid)
+{
+ grub_size_t pos = 0;
+ static const char * def_uuid = PLAINMOUNT_DEFAULT_UUID;
+
+ /* Size of user_uuid is checked in main func */
+ if (user_uuid)
+ grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
+
+ /* Set default UUID. Last digits start from 1 and are incremented for
+ each new plainmount device. */
+ else
+ {
+ /* Set uuid to ' ...x' where x starts from 1. */
+ grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%32lu", dev->id+1);
+ while (dev->uuid[pos++] == ' ');
+ grub_memcpy (dev->uuid, def_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 sector_size, grub_size_t offset)
+{
+ grub_disk_addr_t total_sectors;
+ dev->log_sector_size = grub_log2ull(sector_size);
+
+ /* Convert size to sectors */
+ total_sectors = grub_disk_native_sectors (disk);
+ if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
+ return grub_error (GRUB_ERR_BAD_DEVICE,
+ N_("cannot determine disk %s size"), disk->name);
+
+ total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
+ dev->log_sector_size);
+ if (total_sectors == 0)
+ return grub_error (GRUB_ERR_BAD_DEVICE,
+ N_("cannot set specified sector size on disk %s"), disk->name);
+
+ /* Convert offset to sectors */
+ dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
+ if (total_sectors <= dev->offset_sectors)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("specified disk offset is larger than disk size"));
+
+ /* Adjust device size for offset */
+ dev->total_sectors = total_sectors - dev->offset_sectors;
+ grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
+ ", offset_sectors=%"PRIuGRUB_SIZE"\n", dev->log_sector_size,
+ dev->total_sectors, dev->offset_sectors);
+ return GRUB_ERR_NONE;
+}
+
+
+/* Hashes a password into a key and stores it with cipher. */
+static grub_err_t
+plainmount_configure_password (grub_cryptodisk_t dev, grub_disk_t disk, char *hash,
+ grub_uint8_t *key_data, grub_size_t key_size)
+{
+ const gcry_md_spec_t *gcry_hash;
+ grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
+ char *p;
+ unsigned int round, i;
+ unsigned int len, size;
+
+ /* Option -p was not set */
+ if (key_data[0] == '\0')
+ {
+ char *part = grub_partition_get_name (disk->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name,
+ disk->partition != NULL ? "," : "",
+ part != NULL ? part : N_("UNKNOWN"));
+ grub_free (part);
+
+ if (!grub_password_get ((char*)key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE-1))
+ grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading password"));
+ }
+
+ /* Support none (plain) hash */
+ if (grub_strcmp (hash, "plain") == 0)
+ {
+ dev->hash = NULL;
+ return plainmount_setkey (dev, key_data, key_size);
+ }
+
+ /* Check hash */
+ gcry_hash = grub_crypto_lookup_md_by_name (hash);
+ if (!gcry_hash)
+ return grub_error (GRUB_ERR_FILE_NOT_FOUND,
+ N_("couldn't load %s hash"),
+ hash);
+
+ if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("hash length %"PRIuGRUB_SIZE
+ " exceeds maximum %d bits"),
+ gcry_hash->mdlen * BITS_PER_BYTE,
+ GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
+
+ dev->hash = gcry_hash;
+ len = dev->hash->mdlen;
+ p = grub_malloc (key_size + 2 + key_size / len);
+ 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 keyfile as a file */
+static grub_err_t
+plainmount_configure_keyfile (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
+ grub_size_t key_size, grub_size_t keyfile_offset)
+{
+ grub_file_t g_keyfile = grub_file_open (keyfile, GRUB_FILE_TYPE_NONE);
+ if (!keyfile)
+ return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
+ keyfile);
+
+ if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1)
+ return grub_error (GRUB_ERR_FILE_READ_ERROR,
+ N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
+ keyfile_offset);
+
+ if (key_size > (g_keyfile->size - keyfile_offset))
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("Specified key size (%"PRIuGRUB_SIZE") is too small"
+ " for keyfile size (%"PRIuGRUB_SIZE") and offset (%"
+ PRIuGRUB_SIZE")"),
+ key_size, g_keyfile->size, keyfile_offset);
+
+ if (grub_file_read (g_keyfile, key_data, key_size) != (grub_ssize_t) key_size)
+ return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
+
+ return plainmount_setkey (dev, key_data, key_size);
+}
+
+
+/* Read keyfile as a disk segment */
+static grub_err_t
+plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
+ grub_size_t key_size, grub_size_t keyfile_offset)
+{
+ char *keydisk_name = grub_file_get_device_name (keyfile);
+ grub_disk_t keydisk = grub_disk_open (keyfile);
+ if (!keydisk)
+ {
+ grub_free (keydisk_name);
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unable to open key disk %s"), keydisk_name);
+ }
+ if (grub_disk_read (keydisk, 0, keyfile_offset, key_size, key_data) != GRUB_ERR_NONE)
+ {
+ grub_free (keydisk_name);
+ grub_disk_close (keydisk);
+ return grub_error (GRUB_ERR_READ_ERROR, N_("failed to read key from disk %s"), keydisk_name);
+ }
+ return plainmount_setkey (dev, key_data, key_size);
+}
+
+
+/* Plainmount command entry point */
+static grub_err_t
+grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char **args)
+{
+ struct grub_arg_list *state = ctxt->state;
+ struct grub_plainmount_args cargs = {0};
+ grub_cryptodisk_t dev = NULL;
+ char *diskname, *disklast = NULL;
+ grub_size_t len;
+ grub_err_t err;
+ const char *p;
+
+ if (argc < 1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
+
+ /* Check whether required arguments are specified */
+ if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key size must be set");
+ if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm must be set");
+
+ /* Check cipher mode */
+ if (!grub_strchr (state[OPTION_CIPHER].arg,'-'))
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("invalid cipher mode, must be of format cipher-mode"));
+
+ /* Check password size */
+ if (state[OPTION_PASSWORD].set &&
+ grub_strlen (state[OPTION_PASSWORD].arg) + 1 > GRUB_CRYPTODISK_MAX_PASSPHRASE)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password exceeds maximium size"));
+
+ /* Check uuid length */
+ if (state[OPTION_UUID].set &&
+ grub_strlen (state[OPTION_UUID].arg) > sizeof (PLAINMOUNT_DEFAULT_UUID))
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device UUID exceeds maximum size"));
+
+ /* Parse cmdline arguments */
+ cargs.offset = plainmount_parse_blocklist (args[0]);
+ if (cargs.offset == (grub_size_t)-1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse disk %s"), args[0]);
+ if (state[OPTION_KEYFILE].set)
+ {
+ cargs.keyfile_offset = plainmount_parse_blocklist (state[OPTION_KEYFILE].arg);
+ if (cargs.keyfile_offset == (grub_size_t)-1)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse keyfile %s"),
+ state[OPTION_KEYFILE].arg);
+ }
+
+ grub_errno = GRUB_ERR_NONE;
+ cargs.sector_size = state[OPTION_SECTOR_SIZE].set ?
+ grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, 0) : PLAINMOUNT_DEFAULT_SECTOR_SIZE;
+ if (state[OPTION_SECTOR_SIZE].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
+
+ cargs.key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0) / BITS_PER_BYTE;
+ if (*p != '\0' || grub_errno != GRUB_ERR_NONE)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
+
+ /* Check key size */
+ if (cargs.key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("key size %"PRIuGRUB_SIZE
+ " exceeds maximum %d bits"),
+ cargs.key_size * BITS_PER_BYTE,
+ GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
+
+ /* Check disk sector size */
+ if (cargs.sector_size < 512)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "sector size -S must be at least 512");
+ if ((cargs.sector_size & (cargs.sector_size - 1)) != 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("sector size -S %"PRIuGRUB_SIZE" is not power of 2"),
+ cargs.sector_size);
+
+ /* Allocate all stuff here */
+ cargs.hash = state[OPTION_HASH].set ? grub_strdup (state[OPTION_HASH].arg) : NULL;
+ cargs.cipher = grub_strdup (state[OPTION_CIPHER].arg);
+ cargs.keyfile = state[OPTION_KEYFILE].set ? grub_strdup (state[OPTION_KEYFILE].arg) : NULL;
+ dev = grub_zalloc (sizeof *dev);
+ cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+ cargs.uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) : NULL;
+ if ((!cargs.hash && state[OPTION_HASH].set) || !cargs.cipher ||
+ (!cargs.keyfile && state[OPTION_KEYFILE].set) || !dev || !cargs.key_data ||
+ (!cargs.uuid && state[OPTION_UUID].set))
+ {
+ err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
+ goto exit;
+ }
+
+ /* Copy user password from -p option */
+ if (state[OPTION_PASSWORD].set)
+ grub_memcpy (cargs.key_data, state[OPTION_PASSWORD].arg,
+ grub_strlen (state[OPTION_PASSWORD].arg));
+
+ /* Copy user UUID from -u option */
+ if (state[OPTION_UUID].set)
+ grub_memcpy (cargs.uuid, state[OPTION_UUID].arg, grub_strlen (state[OPTION_UUID].arg));
+
+ /* Set cipher mode (tested above) */
+ cargs.mode = grub_strchr (cargs.cipher,'-');
+ *cargs.mode++ = '\0';
+
+ /* Check cipher */
+ if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
+ {
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("invalid cipher %s"), cargs.cipher);
+ goto exit;
+ }
+
+ /* Open SOURCE disk */
+ diskname = args[0];
+ len = grub_strlen (diskname);
+ if (len && diskname[0] == '(' && diskname[len - 1] == ')')
+ {
+ disklast = &diskname[len - 1];
+ *disklast = '\0';
+ diskname++;
+ }
+ cargs.disk = grub_disk_open (diskname);
+ if (!cargs.disk)
+ {
+ if (disklast)
+ *disklast = ')';
+ err = grub_error (GRUB_ERR_BAD_ARGUMENT,
+ N_("cannot open disk %s"), diskname);
+ goto exit;
+ }
+
+ /* Warn if hash and keyfile are both provided */
+ if (cargs.keyfile && state[OPTION_HASH].arg)
+ grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
+
+ /* Warn if -p option is specified with keyfile */
+ if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
+ grub_printf_ (N_("warning: password specified with -p option "
+ "is ignored if keyfile is provided\n"));
+
+ grub_dprintf ("plainmount",
+ "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE
+ ", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
+ cargs.cipher, cargs.hash, cargs.key_size,
+ cargs.keyfile, cargs.keyfile_offset);
+
+ err = plainmount_configure_sectors (dev, cargs.disk, cargs.sector_size, cargs.offset);
+ if (err != GRUB_ERR_NONE)
+ goto exit;
+
+ /* Configure keyfile/keydisk/password */
+ if (cargs.keyfile)
+ if (cargs.keyfile[0] == '/' ||
+ (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
+ err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
+ cargs.key_size, cargs.keyfile_offset);
+ else
+ err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
+ cargs.key_size, cargs.keyfile_offset);
+ else
+ err = plainmount_configure_password (dev, cargs.disk, cargs.hash,
+ cargs.key_data, cargs.key_size);
+ if (err != GRUB_ERR_NONE)
+ goto exit;
+
+ err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
+ if (err != GRUB_ERR_NONE)
+ {
+ grub_printf_ (N_("cannot initialize cryptodisk. Check consistency "
+ "of cipher/key size/hash arguments.\n"));
+ goto exit;
+ }
+
+ dev->modname = "plainmount";
+ dev->source_disk = cargs.disk;
+ plainmount_set_uuid (dev, cargs.uuid);
+
+exit:
+ grub_free (cargs.hash);
+ grub_free (cargs.cipher);
+ grub_free (cargs.keyfile);
+ grub_free (cargs.key_data);
+ grub_free (cargs.uuid);
+ if (err != GRUB_ERR_NONE && cargs.disk)
+ grub_disk_close (cargs.disk);
+ if (err != GRUB_ERR_NONE && dev)
+ grub_free (dev);
+ return err;
+}
+
+static grub_extcmd_t cmd;
+GRUB_MOD_INIT (plainmount)
+{
+ cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
+ N_("-c cipher -s key-size [-h hash] "
+ " [-S sector-size] [-p password] [-u uuid] "
+ " [-d keyfile] <SOURCE>"),
+ N_("Open partition encrypted in plain mode."),
+ options);
+}
+
+GRUB_MOD_FINI (plainmount)
+{
+ grub_unregister_extcmd (cmd);
+}
--
2.36.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-18 14:33 [PATCH v3 1/1] plainmount: Support plain encryption mode Maxim Fomin
@ 2022-06-25 0:55 ` Glenn Washburn
2022-06-25 11:19 ` Maxim Fomin
2022-06-26 13:37 ` Maxim Fomin
0 siblings, 2 replies; 9+ messages in thread
From: Glenn Washburn @ 2022-06-25 0:55 UTC (permalink / raw)
To: Maxim Fomin; +Cc: grub-devel, ps
On Sat, 18 Jun 2022 14:33:48 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> From 87af7e9cbeb72c3bc146564f64aa8132c1bf6d68 Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sat, 18 Jun 2022 14:32:49 +0100
> Subject: [PATCH v3 1/1] plainmount: Support plain encryption mode.
>
> This patch adds support for plain encryption mode (plain dm-crypt) via
> new module/command named 'plainmount'.
>
There should be a line with only "---" here so that the Changes lines
do not get added as part of the commit message.
> Changes from the second version of the patch:
> - added support for blocklist syntax offset
> - added explicit error for unsupported offset syntax
> - removed offset options -o and -O
I think it makes sense to keep -O because that's the only way to do a
byte offset into the keyfile (without the special "blocklist" parsing),
just as in cryptomount.
> - removed disk size -b option
> - added static compile-time uuid
> - removed 4096 sector size limitation
> - parameters are passed directly (not inside struct)
> - added support for plain hash
> - minor code improvements
> - improved documentation
> ---
> docs/grub.texi | 54 ++++
> grub-core/Makefile.core.def | 5 +
> grub-core/disk/plainmount.c | 517 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 576 insertions(+)
> create mode 100644 grub-core/disk/plainmount.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 9b902273c..a72475a8b 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -4229,6 +4229,7 @@ you forget a command, you can run the command @command{help}
> * parttool:: Modify partition table entries
> * password:: Set a clear-text password
> * password_pbkdf2:: Set a hashed password
> +* plainmount:: Open device encrypted in plain mode.
> * play:: Play a tune
> * probe:: Retrieve device info
> * rdmsr:: Read values from model-specific registers
> @@ -4509,6 +4510,9 @@ function is supported, as Argon2 is not yet supported.
>
> Also, note that, unlike filesystem UUIDs, UUIDs for encrypted devices must be
> specified without dash separators.
> +
> +Support for plain encryption mode (plain dm-crypt) is provided via separate
> +plainmount command.
> @end deffn
>
> @node cutmem
> @@ -5017,6 +5021,56 @@ to generate password hashes. @xref{Security}.
> @end deffn
>
>
> +@node plainmount
> +@subsection plainmount
> +
> +@deffn Command plainmount device @option{-c} cipher @option{-s} key size [@option{-h} hash]
> +[@option{-S} sector size] [@option{-p} password] [@option{-d} keyfile] [@option{-u} uuid]
> +
> +Setup access to encrypted in plain mode device. Password can be specified in
"Setup access to encrypted device in plain mode."
> +two ways: as a secret passphrase or as a byte array from a keyfile. Secret
"keyfile. The secret passphrase"
There are a quite a few articles (eg. "the" or "a") missing here. I've
not listed them all. This is not a criticism, as this is in general
well written, but I'm noting it so we can fix it if desired. I don't
think it makes anything confusing as is, it just is slightly noticeable
that its written by a non-native speaker. So I don't mind it as is.
> +passphrase can be provided interactively from console or with option @option{-p}.
"the option"
> +The keyfile is specified with option @option{-d} and can be either a path to a
> +regular file or a block device. The keyfile parameter @option{-d} has higher
Take out "or a block device"
> +priority than option @option{-p}.
> +
> +Cipher @option{-c} and keysize @option{-s} options are mandatory. Hash option
> +@option{-h} is mandatory if keyfile is not specified (hash can be 'plain' which
> +means no hashing). Cipher must be specified with mode (for example, 'aes-xts-plain64').
> +Key size must not exceed 128 bytes and must be specified in bits (for example,
> +-s 256 or -s 512).
> +
> +Option @option{-S} determines encrypted disk sector size. It should be at least
> +512 bytes and a power of 2. Note: current implementation of cryptsetup allows
> +only 512/ 1024/2048/4096 sectors. Disk sector size in plain mode is set at encryption
> +time. Attempt to decrypt already created device with diffferent sector size will
> +result in error.
I would prefer the last two sentences above to instead be:
Disk sector size is configured when creating the encrypted volume.
Attempting to decrypt a volume with a different sector size than it
was created with will not result in an error, but will decrypt to
random bytes and thus prevent accessing the volume.
> +
> +Offset of encrypted data (device argument) and offset of keyfile (option @option{-d})
> +can be specified with grub blocklist syntax ('+10M') where single word suffixes 'K',
> +'M' and 'G' (base 1024) are available. Note: unlike other grub commands, plainmount
> +supports only single blocklist offset.
Hmm, I wasn't suggesting this be added. I hope you didn't think I was
suggesting this. What I was suggesting was that the block list syntax
already supported in GRUB for device paths be used, not creating a new
block list syntax just for this command. You shouldn't need to add any
new code for what I was suggesting.
For instance, if you know that your plain mount volume is on device
(hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
the key material is offset 35235 bytes into that file you would use:
loopback cplain0 (hd0)2048+
plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
then use:
plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
or
plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
Here the '+' is needed after (hd1) to turn it into a file because -d
should only take a file. It would be nice to have (hd1) be treated as
(hd1)+ when used as a file, but that would be a different patch.
The drawback to what I'm suggesting is that you can't do "-d
(hd1)16K+". This could be something interesting to add to GRUB
blocklist syntax, but as a separate patch.
I believe there's also a confusion here on the usage of blocklist
syntax. Blocklist syntax is about specifying a range of blocks, not an
offset or specific block number. So for instance, "(hd1)+16" means
blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
latter is what you want.
> +
> +By default new cryptodisk node will have uuid '109fea84-a6b7-34a8-4bd1-1c506305a4e1'
> +where last digits are incremented for each subsequently created node. Custom
> +uuid can be specified with option @option{-u}.
> +
> +The plainmount command can report internal cryptographic errors. If they happen,
Its not clear to me what internal cryptographic errors are reported,
under what conditions.
> +check hash, key size and cipher options - they should match each other.
"they should match the parameters used to create the volume."
> +Successfully decrypted disks are named as (cryptoX) and have linear numeration
> +with other decrypted devices opened by cryptodisk.
> +
> +Note, all encryption arguments (cipher, hash, key size, disk offset and disk
> +sector size) must match those which were used to setup encrypted device. If
> +any of them does not match the actual arguments used during the initial encryption,
> +plainmount will return garbage data and GRUB will report unknown filesystem for the
> +opened disk. Note, writing data to such a virtual device will result in data
> +loss if the underlying partition contained desired data. If the encrypted disk hosts
> +some high level abstraction (like LVM2 or MDRAID) it will be created under
> +separate device name.
> +@end deffn
> +
> +
> @node play
> @subsection play
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 715994872..579acd6c7 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1189,6 +1189,11 @@ module = {
> common = disk/luks.c;
> };
>
> +module = {
> + name = plainmount;
> + common = disk/plainmount.c;
> +};
> +
This should be moved to after the "cryptodisk" module.
> module = {
> name = luks2;
> common = disk/luks2.c;
> diff --git a/grub-core/disk/plainmount.c b/grub-core/disk/plainmount.c
> new file mode 100644
> index 000000000..086e5cb50
> --- /dev/null
> +++ b/grub-core/disk/plainmount.c
> @@ -0,0 +1,517 @@
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2003,2007,2010,2011,2019 Free Software Foundation, Inc.
> + *
> + * GRUB is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * GRUB is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +
> +#include <grub/cryptodisk.h>
> +#include <grub/dl.h>
> +#include <grub/err.h>
> +#include <grub/extcmd.h>
> +#include <grub/partition.h>
> +#include <grub/file.h>
> +
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +char PLAINMOUNT_DEFAULT_UUID[] = "109fea84-a6b7-34a8-4bd1-1c506305a4e0";
I'd recommend the last two digits of the default UUID be '0'. This way
we can have up to 256 plain mount devices before creating
non-sequential UUIDs. Right now its 16, which seems like a lot, but
also seems like it could be on the edge of what someone might actually
want to do.
> +#define BITS_PER_BYTE 8
> +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
> +
> +enum PLAINMOUNT_OPTION
> + {
> + OPTION_HASH,
> + OPTION_CIPHER,
> + OPTION_KEY_SIZE,
> + OPTION_SECTOR_SIZE,
> + OPTION_PASSWORD,
> + OPTION_KEYFILE,
> + OPTION_UUID
> + };
> +
> +static const struct grub_arg_option options[] =
> + {
> + /* TRANSLATORS: It's still restricted to this module only. */
> + {"hash", 'h', 0, N_("Password hash"), 0, ARG_TYPE_STRING},
> + {"cipher", 'c', 0, N_("Password cipher"), 0, ARG_TYPE_STRING},
> + {"key-size", 's', 0, N_("Key size (in bits)"), 0, ARG_TYPE_INT},
> + {"sector-size", 'S', 0, N_("Device sector size"), 0, ARG_TYPE_INT},
> + {"password", 'p', 0, N_("Password (key)"), 0, ARG_TYPE_STRING},
> + {"keyfile", 'd', 0, N_("Keyfile/disk path"), 0, ARG_TYPE_STRING},
> + {"uuid", 'u', 0, N_("Set device UUID"), 0, ARG_TYPE_STRING},
> + {0, 0, 0, 0, 0, 0}
> + };
> +
> +struct grub_plainmount_args
> +{
> + grub_uint8_t *key_data;
> + char *cipher, *mode, *hash, *keyfile, *uuid;
> + grub_size_t offset, key_size, sector_size, keyfile_offset;
> + grub_disk_t disk;
> +};
> +typedef struct grub_plainmount_args *grub_plainmount_args_t;
I thought we were going to get rid of this?
> +
> +
> +/* Cryptodisk setkey() function wrapper */
> +static grub_err_t
> +plainmount_setkey (grub_cryptodisk_t dev, grub_uint8_t *data,
> + grub_size_t size)
> +{
> + gcry_err_code_t code = grub_cryptodisk_setkey (dev, data, size);
> + if (code != GPG_ERR_NO_ERROR)
> + {
> + grub_dprintf ("plainmount", "password crypto status is %d\n", code);
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot set key. Check consistency of "
> + "keysize/hash/cipher options."));
I don't really like this because grub_cryptodisk_setkey() should set
the error and we just pass it along. grub_cryptodisk_setkey() should
return a grub_err_t and not a gcry_err_code_t. Since it doesn't do
that, this is reasonable.
> + }
> + return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Parse disk size suffix */
> +static grub_size_t plainmount_parse_suffix (char *arg)
This should be removed. And possibly used in a separate patch for
adding this feature to blocklists in general, but it has issue as is as
well.
> +{
> + const char *p;
> + grub_size_t val = grub_strtoull (arg, &p, 0);
> +
> + /* Only single word prefix is allowed */
> + if (p[1] != '\0' || (p[0] != '\0' && p[0] != 'K' && p[0] != 'k' &&
If p[0] == '\0', then p[1] is an out of bounds access.
> + p[0] != 'M' && p[0] != 'm' && p[0] != 'G' && p[0] != 'g'))
> + return -1;
Perhaps set grub_errno here with grub_error(), but still return -1.
> +
> + switch (*p)
> + {
> + case 'K':
> + case 'k':
> + val = val * 1024;
> + break;
> + case 'M':
> + case 'm':
> + val = val * 1024*1024;
> + break;
> + case 'G':
> + case 'g':
> + val = val * 1024*1024*1024;
> + break;
> + case '\0':
> + break;
> + default:
> + val = -1;
> + }
> + return val;
> +}
> +
> +/* Support blocklist syntax */
> +static grub_size_t plainmount_parse_blocklist (char *arg)
This function should be gotten rid of too.
> +{
> + char *pos = grub_strchr (arg, '+');
Filenames can have the '+' character on many file systems, so this is
not reliable.
> +
> + if (!pos)
> + return 0;
> +
> + /* Erase '+', arg now points to disk, pos+1 to offset */
> + pos[0] = '\0';
> +
> + /* 'hd0,gpt2+' equals 'hd0,gpt2' equals no offset */
> + if (pos[1] == '\0')
> + return 0;
> +
> + /* Blocklist syntax here supports K/M/G suffix */
> + return plainmount_parse_suffix (pos+1);
As noted above, you're matching a number after the '+', which isn't
really correct.
> +}
> +
> +/* Configure cryptodisk uuid */
> +static void plainmount_set_uuid (grub_cryptodisk_t dev, char *user_uuid)
> +{
> + grub_size_t pos = 0;
> + static const char * def_uuid = PLAINMOUNT_DEFAULT_UUID;
> +
> + /* Size of user_uuid is checked in main func */
> + if (user_uuid)
> + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> +
> + /* Set default UUID. Last digits start from 1 and are incremented for
> + each new plainmount device. */
> + else
> + {
> + /* Set uuid to ' ...x' where x starts from 1. */
> + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%32lu", dev->id+1);
The format type specifier should be 'x' instead of 'u' if you want
counting not to skip some UUIDs after the 10th mounted crypto devices.
Also, the format string should be "%36lx", if you want to create a 128
bit UUID. As it is right now, you're only writing a UUID with 112 bits
because PLAINMOUNT_DEFAULT_UUID has dashes which contribute not bits to
the UUID.
> + while (dev->uuid[pos++] == ' ');
> + grub_memcpy (dev->uuid, def_uuid, pos-1);
I'd say get rid of the "def_uuid" variable and just use
PLAINMOUNT_DEFAULT_UUID here.
> + }
> + COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof (PLAINMOUNT_DEFAULT_UUID));
> +}
> +
> +
> +/* Configure cryptodevice sector size (-S option) */
This comment should also note that offset is also configured. But
actually, I don't think we need to do any configuring based on offset
and should just take that code out below. If the user needs an offset,
put that into a loopback device as in the examples I gave above.
> +static grub_err_t
> +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t disk,
> + grub_size_t sector_size, grub_size_t offset)
> +{
> + grub_disk_addr_t total_sectors;
> + dev->log_sector_size = grub_log2ull(sector_size);
Nit, space before '('.
> +
> + /* Convert size to sectors */
> + total_sectors = grub_disk_native_sectors (disk);
> + if (total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> + return grub_error (GRUB_ERR_BAD_DEVICE,
> + N_("cannot determine disk %s size"), disk->name);
> +
> + total_sectors = grub_convert_sector (total_sectors, GRUB_DISK_SECTOR_BITS,
> + dev->log_sector_size);
> + if (total_sectors == 0)
> + return grub_error (GRUB_ERR_BAD_DEVICE,
> + N_("cannot set specified sector size on disk %s"), disk->name);
> +
> + /* Convert offset to sectors */
> + dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
> + if (total_sectors <= dev->offset_sectors)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("specified disk offset is larger than disk size"));
> +
> + /* Adjust device size for offset */
> + dev->total_sectors = total_sectors - dev->offset_sectors;
> + grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%"PRIuGRUB_SIZE
> + ", offset_sectors=%"PRIuGRUB_SIZE"\n", dev->log_sector_size,
> + dev->total_sectors, dev->offset_sectors);
> + return GRUB_ERR_NONE;
> +}
> +
> +
> +/* Hashes a password into a key and stores it with cipher. */
> +static grub_err_t
> +plainmount_configure_password (grub_cryptodisk_t dev, grub_disk_t disk, char *hash,
> + grub_uint8_t *key_data, grub_size_t key_size)
> +{
> + const gcry_md_spec_t *gcry_hash;
> + grub_uint8_t derived_hash[GRUB_CRYPTODISK_MAX_KEYLEN * 2], *dh = derived_hash;
> + char *p;
> + unsigned int round, i;
> + unsigned int len, size;
> +
> + /* Option -p was not set */
> + if (key_data[0] == '\0')
> + {
> + char *part = grub_partition_get_name (disk->partition);
> + grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name,
> + disk->partition != NULL ? "," : "",
> + part != NULL ? part : N_("UNKNOWN"));
> + grub_free (part);
> +
> + if (!grub_password_get ((char*)key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE-1))
> + grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading password"));
> + }
> +
> + /* Support none (plain) hash */
> + if (grub_strcmp (hash, "plain") == 0)
> + {
> + dev->hash = NULL;
> + return plainmount_setkey (dev, key_data, key_size);
> + }
> +
> + /* Check hash */
> + gcry_hash = grub_crypto_lookup_md_by_name (hash);
> + if (!gcry_hash)
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> + N_("couldn't load %s hash"),
> + hash);
> +
> + if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("hash length %"PRIuGRUB_SIZE
> + " exceeds maximum %d bits"),
> + gcry_hash->mdlen * BITS_PER_BYTE,
> + GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
> +
> + dev->hash = gcry_hash;
> + len = dev->hash->mdlen;
> + p = grub_malloc (key_size + 2 + key_size / len);
I think its a little better to have parentheses around "key_size / len".
> + if (!p)
> + return GRUB_ERR_OUT_OF_MEMORY;
> +
> + /* Hash password. Adapted from cryptsetup.
> + https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
> + */
Multi-line comments in GRUB are supposed to be like:
/*
* Hash password. Adapted from cryptsetup.
* https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c
*/
> + for (round = 0, size = key_size; size; round++, dh += len, size -= len)
> + {
> + for (i = 0; i < round; i++)
> + p[i] = 'A';
> +
> + grub_strcpy (p + i, (char*)key_data);
> +
> + if (len > size)
> + len = size;
> +
> + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> + }
> + grub_free (p);
> + return plainmount_setkey (dev, derived_hash, key_size);
> +}
> +
> +
> +/* Read keyfile as a file */
> +static grub_err_t
> +plainmount_configure_keyfile (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> + grub_size_t key_size, grub_size_t keyfile_offset)
> +{
> + grub_file_t g_keyfile = grub_file_open (keyfile, GRUB_FILE_TYPE_NONE);
> + if (!keyfile)
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open keyfile %s"),
> + keyfile);
> +
> + if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1)
> + return grub_error (GRUB_ERR_FILE_READ_ERROR,
> + N_("cannot seek keyfile at offset %"PRIuGRUB_SIZE),
> + keyfile_offset);
> +
> + if (key_size > (g_keyfile->size - keyfile_offset))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("Specified key size (%"PRIuGRUB_SIZE") is too small"
> + " for keyfile size (%"PRIuGRUB_SIZE") and offset (%"
> + PRIuGRUB_SIZE")"),
> + key_size, g_keyfile->size, keyfile_offset);
> +
> + if (grub_file_read (g_keyfile, key_data, key_size) != (grub_ssize_t) key_size)
> + return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading key file"));
> +
> + return plainmount_setkey (dev, key_data, key_size);
> +}
> +
> +
> +/* Read keyfile as a disk segment */
> +static grub_err_t
> +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> + grub_size_t key_size, grub_size_t keyfile_offset)
I don't think this function should exist either. Using GRUB's already
existing blocklist syntax (see example above) and with -O for
specifying keyfile offset, we don't need this.
> +{
> + char *keydisk_name = grub_file_get_device_name (keyfile);
> + grub_disk_t keydisk = grub_disk_open (keyfile);
> + if (!keydisk)
> + {
> + grub_free (keydisk_name);
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unable to open key disk %s"), keydisk_name);
> + }
> + if (grub_disk_read (keydisk, 0, keyfile_offset, key_size, key_data) != GRUB_ERR_NONE)
> + {
> + grub_free (keydisk_name);
> + grub_disk_close (keydisk);
> + return grub_error (GRUB_ERR_READ_ERROR, N_("failed to read key from disk %s"), keydisk_name);
> + }
> + return plainmount_setkey (dev, key_data, key_size);
> +}
> +
> +
> +/* Plainmount command entry point */
> +static grub_err_t
> +grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char **args)
> +{
> + struct grub_arg_list *state = ctxt->state;
> + struct grub_plainmount_args cargs = {0};
I thought were were going to gt rid of this? Now cargs is localized to
only this function, so its even less needed.
> + grub_cryptodisk_t dev = NULL;
> + char *diskname, *disklast = NULL;
> + grub_size_t len;
> + grub_err_t err;
> + const char *p;
> +
> + if (argc < 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name required"));
> +
> + /* Check whether required arguments are specified */
> + if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key size must be set");
> + if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm must be set");
> +
> + /* Check cipher mode */
> + if (!grub_strchr (state[OPTION_CIPHER].arg,'-'))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("invalid cipher mode, must be of format cipher-mode"));
> +
> + /* Check password size */
> + if (state[OPTION_PASSWORD].set &&
> + grub_strlen (state[OPTION_PASSWORD].arg) + 1 > GRUB_CRYPTODISK_MAX_PASSPHRASE)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("password exceeds maximium size"));
> +
> + /* Check uuid length */
> + if (state[OPTION_UUID].set &&
> + grub_strlen (state[OPTION_UUID].arg) > sizeof (PLAINMOUNT_DEFAULT_UUID))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device UUID exceeds maximum size"));
> +
> + /* Parse cmdline arguments */
> + cargs.offset = plainmount_parse_blocklist (args[0]);
> + if (cargs.offset == (grub_size_t)-1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse disk %s"), args[0]);
Let's not create new blocklist syntax parsing/handling and instead let
the user use GRUB's already built in syntax while creating a loopback
device. This should allow us to not have to worry about offset at all.
As noted above, if its really desirable to specify encrypted blocks like
"(hd1)16M+", then that should be a separate patch to the already
existing blocklist parser. I'm not opposed to that either, I think it
could be convenient syntax. It just shouldn't be in this patch.
> + if (state[OPTION_KEYFILE].set)
> + {
> + cargs.keyfile_offset = plainmount_parse_blocklist (state[OPTION_KEYFILE].arg);
> + if (cargs.keyfile_offset == (grub_size_t)-1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot parse keyfile %s"),
> + state[OPTION_KEYFILE].arg);
And here, let's go back to setting keyfile_offset based on the -O
parameter. This has the added benefit of being inline with cryptomount
options.
> + }
> +
> + grub_errno = GRUB_ERR_NONE;
> + cargs.sector_size = state[OPTION_SECTOR_SIZE].set ?
> + grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, 0) : PLAINMOUNT_DEFAULT_SECTOR_SIZE;
> + if (state[OPTION_SECTOR_SIZE].set && (*p != '\0' || grub_errno != GRUB_ERR_NONE))
The concensus now seems to be that the proper way to handle errors in
grub_strtoull() is the following:
if (state[OPTION_SECTOR_SIZE].set && (state[OPTION_SECTOR_SIZE].arg
== '\0' || *p != '\0'))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized sector size"));
> +
> + cargs.key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0) / BITS_PER_BYTE;
> + if (*p != '\0' || grub_errno != GRUB_ERR_NONE)
Similar to above comment.
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key size"));
> +
> + /* Check key size */
> + if (cargs.key_size > GRUB_CRYPTODISK_MAX_KEYLEN)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("key size %"PRIuGRUB_SIZE
> + " exceeds maximum %d bits"),
> + cargs.key_size * BITS_PER_BYTE,
> + GRUB_CRYPTODISK_MAX_KEYLEN * BITS_PER_BYTE);
> +
> + /* Check disk sector size */
> + if (cargs.sector_size < 512)
s/512/GRUB_DISK_SECTOR_SIZE/
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "sector size -S must be at least 512");
Parameterize 512 in this string and use GRUB_DISK_SECTOR_SIZE.
> + if ((cargs.sector_size & (cargs.sector_size - 1)) != 0)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("sector size -S %"PRIuGRUB_SIZE" is not power of 2"),
> + cargs.sector_size);
> +
> + /* Allocate all stuff here */
> + cargs.hash = state[OPTION_HASH].set ? grub_strdup (state[OPTION_HASH].arg) : NULL;
> + cargs.cipher = grub_strdup (state[OPTION_CIPHER].arg);
> + cargs.keyfile = state[OPTION_KEYFILE].set ? grub_strdup (state[OPTION_KEYFILE].arg) : NULL;
> + dev = grub_zalloc (sizeof *dev);
> + cargs.key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + cargs.uuid = state[OPTION_UUID].set ? grub_strdup (state[OPTION_UUID].arg) : NULL;
> + if ((!cargs.hash && state[OPTION_HASH].set) || !cargs.cipher ||
> + (!cargs.keyfile && state[OPTION_KEYFILE].set) || !dev || !cargs.key_data ||
> + (!cargs.uuid && state[OPTION_UUID].set))
> + {
> + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
> + goto exit;
> + }
> +
> + /* Copy user password from -p option */
> + if (state[OPTION_PASSWORD].set)
> + grub_memcpy (cargs.key_data, state[OPTION_PASSWORD].arg,
> + grub_strlen (state[OPTION_PASSWORD].arg));
> +
> + /* Copy user UUID from -u option */
> + if (state[OPTION_UUID].set)
> + grub_memcpy (cargs.uuid, state[OPTION_UUID].arg, grub_strlen (state[OPTION_UUID].arg));
> +
> + /* Set cipher mode (tested above) */
> + cargs.mode = grub_strchr (cargs.cipher,'-');
> + *cargs.mode++ = '\0';
> +
> + /* Check cipher */
> + if (grub_cryptodisk_setcipher (dev, cargs.cipher, cargs.mode) != GRUB_ERR_NONE)
> + {
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("invalid cipher %s"), cargs.cipher);
> + goto exit;
> + }
> +
> + /* Open SOURCE disk */
> + diskname = args[0];
> + len = grub_strlen (diskname);
> + if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> + {
> + disklast = &diskname[len - 1];
> + *disklast = '\0';
> + diskname++;
> + }
> + cargs.disk = grub_disk_open (diskname);
> + if (!cargs.disk)
> + {
> + if (disklast)
> + *disklast = ')';
> + err = grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("cannot open disk %s"), diskname);
> + goto exit;
> + }
> +
> + /* Warn if hash and keyfile are both provided */
> + if (cargs.keyfile && state[OPTION_HASH].arg)
> + grub_printf_ (N_("warning: hash is ignored if keyfile is specified\n"));
> +
> + /* Warn if -p option is specified with keyfile */
> + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set)
> + grub_printf_ (N_("warning: password specified with -p option "
> + "is ignored if keyfile is provided\n"));
> +
> + grub_dprintf ("plainmount",
> + "parameters: cipher=%s, hash=%s, key_size=%"PRIuGRUB_SIZE
> + ", keyfile=%s, keyfile offset=%"PRIuGRUB_SIZE"\n",
> + cargs.cipher, cargs.hash, cargs.key_size,
> + cargs.keyfile, cargs.keyfile_offset);
> +
> + err = plainmount_configure_sectors (dev, cargs.disk, cargs.sector_size, cargs.offset);
> + if (err != GRUB_ERR_NONE)
> + goto exit;
> +
> + /* Configure keyfile/keydisk/password */
> + if (cargs.keyfile)
> + if (cargs.keyfile[0] == '/' ||
> + (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> + err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> + cargs.key_size, cargs.keyfile_offset);
> + else
> + err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> + cargs.key_size, cargs.keyfile_offset);
We shouldn't support sending a device as a keyfile and only support
files. As noted above, if the keyfile data is only accessibly via some
blocks on a disk device, then use the builtin blocklist syntax
potentially with the -O keyfile offset.
> + else
> + err = plainmount_configure_password (dev, cargs.disk, cargs.hash,
> + cargs.key_data, cargs.key_size);
> + if (err != GRUB_ERR_NONE)
> + goto exit;
> +
> + err = grub_cryptodisk_insert (dev, diskname, cargs.disk);
> + if (err != GRUB_ERR_NONE)
> + {
> + grub_printf_ (N_("cannot initialize cryptodisk. Check consistency "
> + "of cipher/key size/hash arguments.\n"));
This error message will only get printed when grub_cryptodisk_insert()
fails, which only happens when the grub_strdup() that is calls fails.
Thus this error message does not fit the error condition. This error
has nothing to do with the correctness of "cipher/key size/hash
arguments", but is instead an out of memory error.
> + goto exit;
> + }
> +
> + dev->modname = "plainmount";
> + dev->source_disk = cargs.disk;
> + plainmount_set_uuid (dev, cargs.uuid);
> +
> +exit:
> + grub_free (cargs.hash);
> + grub_free (cargs.cipher);
> + grub_free (cargs.keyfile);
> + grub_free (cargs.key_data);
> + grub_free (cargs.uuid);
> + if (err != GRUB_ERR_NONE && cargs.disk)
> + grub_disk_close (cargs.disk);
> + if (err != GRUB_ERR_NONE && dev)
> + grub_free (dev);
> + return err;
> +}
> +
> +static grub_extcmd_t cmd;
> +GRUB_MOD_INIT (plainmount)
> +{
> + cmd = grub_register_extcmd ("plainmount", grub_cmd_plainmount, 0,
> + N_("-c cipher -s key-size [-h hash] "
> + " [-S sector-size] [-p password] [-u uuid] "
> + " [-d keyfile] <SOURCE>"),
> + N_("Open partition encrypted in plain mode."),
> + options);
> +}
> +
> +GRUB_MOD_FINI (plainmount)
> +{
> + grub_unregister_extcmd (cmd);
> +}
> --
> 2.36.1
>
Glenn
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-25 0:55 ` Glenn Washburn
@ 2022-06-25 11:19 ` Maxim Fomin
2022-06-26 13:37 ` Maxim Fomin
1 sibling, 0 replies; 9+ messages in thread
From: Maxim Fomin @ 2022-06-25 11:19 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn <development@efficientek.com> wrote:
> > +
> > +Offset of encrypted data (device argument) and offset of keyfile (option @option{-d})
> > +can be specified with grub blocklist syntax ('+10M') where single word suffixes 'K',
> > +'M' and 'G' (base 1024) are available. Note: unlike other grub commands, plainmount
> > +supports only single blocklist offset.
>
>
> Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> suggesting this. What I was suggesting was that the block list syntax
> already supported in GRUB for device paths be used, not creating a new
> block list syntax just for this command. You shouldn't need to add any
> new code for what I was suggesting.
>
> For instance, if you know that your plain mount volume is on device
> (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> the key material is offset 35235 bytes into that file you would use:
>
> loopback cplain0 (hd0)2048+
> plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
>
> If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> then use:
>
> plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
>
> or
>
> plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
>
> Here the '+' is needed after (hd1) to turn it into a file because -d
> should only take a file. It would be nice to have (hd1) be treated as
> (hd1)+ when used as a file, but that would be a different patch.
>
> The drawback to what I'm suggesting is that you can't do "-d
> (hd1)16K+". This could be something interesting to add to GRUB
> blocklist syntax, but as a separate patch.
>
> I believe there's also a confusion here on the usage of blocklist
> syntax. Blocklist syntax is about specifying a range of blocks, not an
> offset or specific block number. So for instance, "(hd1)+16" means
> blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> latter is what you want.
I thought that the blocklist syntax is some specification which is implemented in
each command because I didn't use that feature. Now I understand you and will
remove not needed parsing functions mentioned below and resubmit the patch.
> > +By default new cryptodisk node will have uuid '109fea84-a6b7-34a8-4bd1-1c506305a4e1'
> > +where last digits are incremented for each subsequently created node. Custom
> > +uuid can be specified with option @option{-u}.
> > +
> > +The plainmount command can report internal cryptographic errors. If they happen,
>
>
> Its not clear to me what internal cryptographic errors are reported,
> under what conditions.
I was refering to the possible error in cryptodisk_setkey() function. When I was
debugging first versions of the patch I have noticed that when cipher/key size
do not match each other or have invalid values, setkey() returns error.
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-25 0:55 ` Glenn Washburn
2022-06-25 11:19 ` Maxim Fomin
@ 2022-06-26 13:37 ` Maxim Fomin
2022-06-26 21:20 ` Glenn Washburn
1 sibling, 1 reply; 9+ messages in thread
From: Maxim Fomin @ 2022-06-26 13:37 UTC (permalink / raw)
To: development, grub-devel
------- Original Message -------
On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn <development@efficientek.com> wrote:
>
> Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> suggesting this. What I was suggesting was that the block list syntax
> already supported in GRUB for device paths be used, not creating a new
> block list syntax just for this command. You shouldn't need to add any
> new code for what I was suggesting.
>
> For instance, if you know that your plain mount volume is on device
> (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> the key material is offset 35235 bytes into that file you would use:
>
> loopback cplain0 (hd0)2048+
> plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
>
> If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> then use:
>
> plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
>
> or
>
> plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
>
> Here the '+' is needed after (hd1) to turn it into a file because -d
> should only take a file. It would be nice to have (hd1) be treated as
> (hd1)+ when used as a file, but that would be a different patch.
>
> The drawback to what I'm suggesting is that you can't do "-d
> (hd1)16K+". This could be something interesting to add to GRUB
> blocklist syntax, but as a separate patch.
>
> I believe there's also a confusion here on the usage of blocklist
> syntax. Blocklist syntax is about specifying a range of blocks, not an
> offset or specific block number. So for instance, "(hd1)+16" means
> blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> latter is what you want.
>
...
> > +/ Read keyfile as a disk segment */
> > +static grub_err_t
> > +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> > + grub_size_t key_size, grub_size_t keyfile_offset)
>
>
> I don't think this function should exist either. Using GRUB's already
> existing blocklist syntax (see example above) and with -O for
> specifying keyfile offset, we don't need this.
>
...
> > + / Configure keyfile/keydisk/password */
> > + if (cargs.keyfile)
> > + if (cargs.keyfile[0] == '/' ||
> > + (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> > + err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> > + cargs.key_size, cargs.keyfile_offset);
> > + else
> > + err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> > + cargs.key_size, cargs.keyfile_offset);
>
>
> We shouldn't support sending a device as a keyfile and only support
> files. As noted above, if the keyfile data is only accessibly via some
> blocks on a disk device, then use the builtin blocklist syntax
> potentially with the -O keyfile offset.
>
>
> Glenn
I don't quite understand this. Irrespective of how device argument is sent (and syntax used),
processing device blocks in 'configure_keyfile()' differes from processing a file. I tested
grub_file_open() on a loopback device and it does not work. It makes sense, because neither
'(hdx,gpty)NNN+' nor a loopback node on top of it is a file. So, I think that supporting
blocks on disk requires some additional code in 'configure_keyfile()'. Perhaps you mean moving
'configure_keydisk()' code inside 'plainmount_configure_keyfile()' and removing it definition?
Or I am missing something?
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-26 13:37 ` Maxim Fomin
@ 2022-06-26 21:20 ` Glenn Washburn
2022-06-28 5:07 ` Maxim Fomin
0 siblings, 1 reply; 9+ messages in thread
From: Glenn Washburn @ 2022-06-26 21:20 UTC (permalink / raw)
To: Maxim Fomin; +Cc: grub-devel
On Sun, 26 Jun 2022 13:37:07 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
> On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn <development@efficientek.com> wrote:
> >
> > Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> > suggesting this. What I was suggesting was that the block list syntax
> > already supported in GRUB for device paths be used, not creating a new
> > block list syntax just for this command. You shouldn't need to add any
> > new code for what I was suggesting.
> >
> > For instance, if you know that your plain mount volume is on device
> > (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> > the key material is offset 35235 bytes into that file you would use:
> >
> > loopback cplain0 (hd0)2048+
> > plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
> >
> > If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> > then use:
> >
> > plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
> >
> > or
> >
> > plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
> >
> > Here the '+' is needed after (hd1) to turn it into a file because -d
> > should only take a file. It would be nice to have (hd1) be treated as
> > (hd1)+ when used as a file, but that would be a different patch.
> >
> > The drawback to what I'm suggesting is that you can't do "-d
> > (hd1)16K+". This could be something interesting to add to GRUB
> > blocklist syntax, but as a separate patch.
> >
> > I believe there's also a confusion here on the usage of blocklist
> > syntax. Blocklist syntax is about specifying a range of blocks, not an
> > offset or specific block number. So for instance, "(hd1)+16" means
> > blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> > the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> > latter is what you want.
> >
>
> ...
>
> > > +/ Read keyfile as a disk segment */
> > > +static grub_err_t
> > > +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> > > + grub_size_t key_size, grub_size_t keyfile_offset)
> >
> >
> > I don't think this function should exist either. Using GRUB's already
> > existing blocklist syntax (see example above) and with -O for
> > specifying keyfile offset, we don't need this.
> >
>
> ...
>
> > > + / Configure keyfile/keydisk/password */
> > > + if (cargs.keyfile)
> > > + if (cargs.keyfile[0] == '/' ||
> > > + (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> > > + err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> > > + cargs.key_size, cargs.keyfile_offset);
> > > + else
> > > + err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> > > + cargs.key_size, cargs.keyfile_offset);
> >
> >
> > We shouldn't support sending a device as a keyfile and only support
> > files. As noted above, if the keyfile data is only accessibly via some
> > blocks on a disk device, then use the builtin blocklist syntax
> > potentially with the -O keyfile offset.
> >
> >
> > Glenn
>
> I don't quite understand this. Irrespective of how device argument is sent (and syntax used),
> processing device blocks in 'configure_keyfile()' differes from processing a file. I tested
This isn't making sense to me. The function
plainmount_configure_keyfile(), which I presume you are referring to
above, uses grub_file_open(), so it expects a file-type argument (which
is a (dev)/path/to/file path or (dev)N+M blocklist). How does this
differ from processing a file?
> grub_file_open() on a loopback device and it does not work. It makes sense, because neither
> '(hdx,gpty)NNN+' nor a loopback node on top of it is a file. So, I think that supporting
Yes, grub_file_open() does not open raw devices (although I think it
should). However, you also seem to say that '(hdx,gpty)NNN+' is not a
file, which I take to mean that it can not be opened by
grub_file_open(). But look at the source for grub_file_open() in
grub-core/kern/file.c (search for the comment with the word
"blocklist"). There you will find that grub_file_open does open
blocklists, so blocklists can be used where file paths are used.
> blocks on disk requires some additional code in 'configure_keyfile()'. Perhaps you mean moving
> 'configure_keydisk()' code inside 'plainmount_configure_keyfile()' and removing it definition?
Nope, I'm saying to get rid of plainmount_configure_keydisk()
completely. I haven't precisely tested this case, so I'm not 100%
certain of the above, but I'm over 90% certain that its true.
For instance, note that the cat command uses grub_file_open() and the
following works: cat (dev)+1.
Glenn
> Or I am missing something?
>
> Best regards,
> Maxim Fomin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-26 21:20 ` Glenn Washburn
@ 2022-06-28 5:07 ` Maxim Fomin
2022-06-28 17:46 ` Glenn Washburn
0 siblings, 1 reply; 9+ messages in thread
From: Maxim Fomin @ 2022-06-28 5:07 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Sunday, June 26th, 2022 at 9:20 PM, Glenn Washburn <development@efficientek.com> wrote:
>
>
> On Sun, 26 Jun 2022 13:37:07 +0000
> Maxim Fomin maxim@fomin.one wrote:
>
> > ------- Original Message -------
> > On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn development@efficientek.com wrote:
> >
> > > Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> > > suggesting this. What I was suggesting was that the block list syntax
> > > already supported in GRUB for device paths be used, not creating a new
> > > block list syntax just for this command. You shouldn't need to add any
> > > new code for what I was suggesting.
> > >
> > > For instance, if you know that your plain mount volume is on device
> > > (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> > > the key material is offset 35235 bytes into that file you would use:
> > >
> > > loopback cplain0 (hd0)2048+
> > > plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
> > >
> > > If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> > > then use:
> > >
> > > plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
> > >
> > > or
> > >
> > > plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
> > >
> > > Here the '+' is needed after (hd1) to turn it into a file because -d
> > > should only take a file. It would be nice to have (hd1) be treated as
> > > (hd1)+ when used as a file, but that would be a different patch.
> > >
> > > The drawback to what I'm suggesting is that you can't do "-d
> > > (hd1)16K+". This could be something interesting to add to GRUB
> > > blocklist syntax, but as a separate patch.
> > >
> > > I believe there's also a confusion here on the usage of blocklist
> > > syntax. Blocklist syntax is about specifying a range of blocks, not an
> > > offset or specific block number. So for instance, "(hd1)+16" means
> > > blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> > > the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> > > latter is what you want.
> >
> > ...
> >
> > > > +/ Read keyfile as a disk segment */
> > > > +static grub_err_t
> > > > +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> > > > + grub_size_t key_size, grub_size_t keyfile_offset)
> > >
> > > I don't think this function should exist either. Using GRUB's already
> > > existing blocklist syntax (see example above) and with -O for
> > > specifying keyfile offset, we don't need this.
> >
> > ...
> >
> > > > + / Configure keyfile/keydisk/password */
> > > > + if (cargs.keyfile)
> > > > + if (cargs.keyfile[0] == '/' ||
> > > > + (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> > > > + err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> > > > + cargs.key_size, cargs.keyfile_offset);
> > > > + else
> > > > + err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> > > > + cargs.key_size, cargs.keyfile_offset);
> > >
> > > We shouldn't support sending a device as a keyfile and only support
> > > files. As noted above, if the keyfile data is only accessibly via some
> > > blocks on a disk device, then use the builtin blocklist syntax
> > > potentially with the -O keyfile offset.
> > >
> > > Glenn
> >
> > I don't quite understand this. Irrespective of how device argument is sent (and syntax used),
> > processing device blocks in 'configure_keyfile()' differes from processing a file. I tested
>
>
> This isn't making sense to me. The function
> plainmount_configure_keyfile(), which I presume you are referring to
> above, uses grub_file_open(), so it expects a file-type argument (which
> is a (dev)/path/to/file path or (dev)N+M blocklist). How does this
> differ from processing a file?
I wanted to say 'configure_keydisk' instead of 'configure_keyfile'. But the comment below shows
you understood my point.
> > grub_file_open() on a loopback device and it does not work. It makes sense, because neither
> > '(hdx,gpty)NNN+' nor a loopback node on top of it is a file. So, I think that supporting
>
>
> Yes, grub_file_open() does not open raw devices (although I think it
> should). However, you also seem to say that '(hdx,gpty)NNN+' is not a
> file, which I take to mean that it can not be opened by
> grub_file_open(). But look at the source for grub_file_open() in
> grub-core/kern/file.c (search for the comment with the word
> "blocklist"). There you will find that grub_file_open does open
> blocklists, so blocklists can be used where file paths are used.
After rechecking this issue it seems 'grub_file_open()' indeed supports blocklist syntax.
> > blocks on disk requires some additional code in 'configure_keyfile()'. Perhaps you mean moving
> > 'configure_keydisk()' code inside 'plainmount_configure_keyfile()' and removing it definition?
>
>
> Nope, I'm saying to get rid of plainmount_configure_keydisk()
> completely. I haven't precisely tested this case, so I'm not 100%
> certain of the above, but I'm over 90% certain that its true.
>
> For instance, note that the cat command uses grub_file_open() and the
> following works: cat (dev)+1.
I will completely remove 'configure_keydisk' as it is not necessary.
One more point - are you sure the '-O' option mentioned in the previous email is really needed if
the keyfile offset can be specified with the blocklist syntax? It looks strange not to have '-o' option for
encrypted device (relying on loopback file with blocklist syntax) but having '-O' option for keyfile offset
(which can be specified with blocklist syntax too - in this case even without loopback).
Also, I am thinking whether it will be easier from the user perspective to support blocklist syntax (without
loopback) for device argument too - having the same syntax for device and offset arguments is more clear.
However, it will works only if 'grub_file_t' provides interface to 'grub_disk_t' object which is needed for
cryptodisk to read encrypted data. I didn't look at it, but if it works, the command syntax can be simplified
to blocklist syntax for both device and offset argument.
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-28 5:07 ` Maxim Fomin
@ 2022-06-28 17:46 ` Glenn Washburn
2022-06-28 21:07 ` Maxim Fomin
0 siblings, 1 reply; 9+ messages in thread
From: Glenn Washburn @ 2022-06-28 17:46 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Tue, 28 Jun 2022 05:07:43 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
> On Sunday, June 26th, 2022 at 9:20 PM, Glenn Washburn <development@efficientek.com> wrote:
> >
> >
> > On Sun, 26 Jun 2022 13:37:07 +0000
> > Maxim Fomin maxim@fomin.one wrote:
> >
> > > ------- Original Message -------
> > > On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn development@efficientek.com wrote:
> > >
> > > > Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> > > > suggesting this. What I was suggesting was that the block list syntax
> > > > already supported in GRUB for device paths be used, not creating a new
> > > > block list syntax just for this command. You shouldn't need to add any
> > > > new code for what I was suggesting.
> > > >
> > > > For instance, if you know that your plain mount volume is on device
> > > > (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> > > > the key material is offset 35235 bytes into that file you would use:
> > > >
> > > > loopback cplain0 (hd0)2048+
> > > > plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
> > > >
> > > > If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> > > > then use:
> > > >
> > > > plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
> > > >
> > > > or
> > > >
> > > > plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
> > > >
> > > > Here the '+' is needed after (hd1) to turn it into a file because -d
> > > > should only take a file. It would be nice to have (hd1) be treated as
> > > > (hd1)+ when used as a file, but that would be a different patch.
> > > >
> > > > The drawback to what I'm suggesting is that you can't do "-d
> > > > (hd1)16K+". This could be something interesting to add to GRUB
> > > > blocklist syntax, but as a separate patch.
> > > >
> > > > I believe there's also a confusion here on the usage of blocklist
> > > > syntax. Blocklist syntax is about specifying a range of blocks, not an
> > > > offset or specific block number. So for instance, "(hd1)+16" means
> > > > blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> > > > the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> > > > latter is what you want.
> > >
> > > ...
> > >
> > > > > +/ Read keyfile as a disk segment */
> > > > > +static grub_err_t
> > > > > +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, grub_uint8_t *key_data,
> > > > > + grub_size_t key_size, grub_size_t keyfile_offset)
> > > >
> > > > I don't think this function should exist either. Using GRUB's already
> > > > existing blocklist syntax (see example above) and with -O for
> > > > specifying keyfile offset, we don't need this.
> > >
> > > ...
> > >
> > > > > + / Configure keyfile/keydisk/password */
> > > > > + if (cargs.keyfile)
> > > > > + if (cargs.keyfile[0] == '/' ||
> > > > > + (grub_strchr (cargs.keyfile, ')') < grub_strrchr(cargs.keyfile,'/')))
> > > > > + err = plainmount_configure_keyfile (dev, cargs.keyfile, cargs.key_data,
> > > > > + cargs.key_size, cargs.keyfile_offset);
> > > > > + else
> > > > > + err = plainmount_configure_keydisk (dev, cargs.keyfile, cargs.key_data,
> > > > > + cargs.key_size, cargs.keyfile_offset);
> > > >
> > > > We shouldn't support sending a device as a keyfile and only support
> > > > files. As noted above, if the keyfile data is only accessibly via some
> > > > blocks on a disk device, then use the builtin blocklist syntax
> > > > potentially with the -O keyfile offset.
> > > >
> > > > Glenn
> > >
> > > I don't quite understand this. Irrespective of how device argument is sent (and syntax used),
> > > processing device blocks in 'configure_keyfile()' differes from processing a file. I tested
> >
> >
> > This isn't making sense to me. The function
> > plainmount_configure_keyfile(), which I presume you are referring to
> > above, uses grub_file_open(), so it expects a file-type argument (which
> > is a (dev)/path/to/file path or (dev)N+M blocklist). How does this
> > differ from processing a file?
>
> I wanted to say 'configure_keydisk' instead of 'configure_keyfile'. But the comment below shows
> you understood my point.
>
> > > grub_file_open() on a loopback device and it does not work. It makes sense, because neither
> > > '(hdx,gpty)NNN+' nor a loopback node on top of it is a file. So, I think that supporting
> >
> >
> > Yes, grub_file_open() does not open raw devices (although I think it
> > should). However, you also seem to say that '(hdx,gpty)NNN+' is not a
> > file, which I take to mean that it can not be opened by
> > grub_file_open(). But look at the source for grub_file_open() in
> > grub-core/kern/file.c (search for the comment with the word
> > "blocklist"). There you will find that grub_file_open does open
> > blocklists, so blocklists can be used where file paths are used.
>
> After rechecking this issue it seems 'grub_file_open()' indeed supports blocklist syntax.
>
> > > blocks on disk requires some additional code in 'configure_keyfile()'. Perhaps you mean moving
> > > 'configure_keydisk()' code inside 'plainmount_configure_keyfile()' and removing it definition?
> >
> >
> > Nope, I'm saying to get rid of plainmount_configure_keydisk()
> > completely. I haven't precisely tested this case, so I'm not 100%
> > certain of the above, but I'm over 90% certain that its true.
> >
> > For instance, note that the cat command uses grub_file_open() and the
> > following works: cat (dev)+1.
>
> I will completely remove 'configure_keydisk' as it is not necessary.
>
> One more point - are you sure the '-O' option mentioned in the previous email is really needed if
> the keyfile offset can be specified with the blocklist syntax? It looks strange not to have '-o' option for
> encrypted device (relying on loopback file with blocklist syntax) but having '-O' option for keyfile offset
> (which can be specified with blocklist syntax too - in this case even without loopback).
The reason that we need '-O', or keyfile offset, is because block
syntax is for specifying block ranges, not byte ranges. Blocks are
sized in the native GRUB block size, which is 512 bytes. If we do not
have '-O', then keyfile data must be aligned on 512-byte boundaries,
which I think is an unreasonable restriction.
On the other hand, I don't think that a '-o' option is needed because
restricting plainmount encrypted data to be 512-byte aligned seems a
reasonable restriction. If the data is not 512-byte aligned, I suspect
you're going to get poorer read/write performance. This is maybe not a
problem for small plainmounts that may contain key material for
unlocking other volumes. So I'm open to this being a desirable feature,
and not dead-set against it.
It would be interesting to verify that cryptsetup cannot create a
LUKS1/LUKS2 volume where the data offset is not 512-byte aligned. I
think this is true, but the LUKS2 standard has the offset in bytes, so
its technically possible. If cryptsetup can create a volume where the
encrypted data is not 512-aligned, then there's a good case for adding
'-o'.
> Also, I am thinking whether it will be easier from the user perspective to support blocklist syntax (without
> loopback) for device argument too - having the same syntax for device and offset arguments is more clear.
Do you mean "file argument" instead of "device argument" here? Because
devices already support blocklist syntax.
> However, it will works only if 'grub_file_t' provides interface to 'grub_disk_t' object which is needed for
> cryptodisk to read encrypted data. I didn't look at it, but if it works, the command syntax can be simplified
> to blocklist syntax for both device and offset argument.
I'm not sure I'm understanding this. Can you give some examples of what
the new command syntax that you're thinking of might look like? How
would blocklist syntax be used for the offset argument? That sounds
like it could be confusing.
Glenn
>
> Best regards,
> Maxim Fomin
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-28 17:46 ` Glenn Washburn
@ 2022-06-28 21:07 ` Maxim Fomin
2022-06-30 19:43 ` Glenn Washburn
0 siblings, 1 reply; 9+ messages in thread
From: Maxim Fomin @ 2022-06-28 21:07 UTC (permalink / raw)
To: grub-devel
------- Original Message -------
On Tuesday, June 28th, 2022 at 5:46 PM, Glenn Washburn <development@efficientek.com> wrote:
> The reason that we need '-O', or keyfile offset, is because block
> syntax is for specifying block ranges, not byte ranges. Blocks are
> sized in the native GRUB block size, which is 512 bytes. If we do not
> have '-O', then keyfile data must be aligned on 512-byte boundaries,
> which I think is an unreasonable restriction.
Agreed.
> On the other hand, I don't think that a '-o' option is needed because
> restricting plainmount encrypted data to be 512-byte aligned seems a
> reasonable restriction. If the data is not 512-byte aligned, I suspect
> you're going to get poorer read/write performance. This is maybe not a
> problem for small plainmounts that may contain key material for
> unlocking other volumes. So I'm open to this being a desirable feature,
> and not dead-set against it.
>
> It would be interesting to verify that cryptsetup cannot create a
> LUKS1/LUKS2 volume where the data offset is not 512-byte aligned. I
> think this is true, but the LUKS2 standard has the offset in bytes, so
> its technically possible. If cryptsetup can create a volume where the
> encrypted data is not 512-aligned, then there's a good case for adding
> '-o'.
cryptsetup supports only 512 byte block alignment for LUKS1 and 512-4096 alignment for LUKS2. Its offset parameter is specified in terms of the number of 512 byte blocks.
> > Also, I am thinking whether it will be easier from the user perspective to support blocklist syntax (without
> > loopback) for device argument too - having the same syntax for device and offset arguments is more clear.
>
>
> Do you mean "file argument" instead of "device argument" here? Because
> devices already support blocklist syntax.
>
> > However, it will works only if 'grub_file_t' provides interface to 'grub_disk_t' object which is needed for
> > cryptodisk to read encrypted data. I didn't look at it, but if it works, the command syntax can be simplified
> > to blocklist syntax for both device and offset argument.
>
>
> I'm not sure I'm understanding this. Can you give some examples of what
> the new command syntax that you're thinking of might look like? How
> would blocklist syntax be used for the offset argument? That sounds
> like it could be confusing.
>
> Glenn
>
I was not saying about some new syntax for some device offset argument.
I am speaking about this:
loopback node (hd0,gpt2)2048+
plainmount node -c aes-xts-plain64 -h sha512 // works
plainmount (hd0,gpt2)2048+ -c aes-xts-plain64 -h sha512 // currently - no
Currently the second command does not work without loopback, I am thinking about removing this limitation in plainmount.
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
2022-06-28 21:07 ` Maxim Fomin
@ 2022-06-30 19:43 ` Glenn Washburn
0 siblings, 0 replies; 9+ messages in thread
From: Glenn Washburn @ 2022-06-30 19:43 UTC (permalink / raw)
To: Maxim Fomin; +Cc: The development of GNU GRUB
On Tue, 28 Jun 2022 21:07:47 +0000
Maxim Fomin <maxim@fomin.one> wrote:
> ------- Original Message -------
> On Tuesday, June 28th, 2022 at 5:46 PM, Glenn Washburn <development@efficientek.com> wrote:
> > The reason that we need '-O', or keyfile offset, is because block
> > syntax is for specifying block ranges, not byte ranges. Blocks are
> > sized in the native GRUB block size, which is 512 bytes. If we do not
> > have '-O', then keyfile data must be aligned on 512-byte boundaries,
> > which I think is an unreasonable restriction.
>
> Agreed.
>
> > On the other hand, I don't think that a '-o' option is needed because
> > restricting plainmount encrypted data to be 512-byte aligned seems a
> > reasonable restriction. If the data is not 512-byte aligned, I suspect
> > you're going to get poorer read/write performance. This is maybe not a
> > problem for small plainmounts that may contain key material for
> > unlocking other volumes. So I'm open to this being a desirable feature,
> > and not dead-set against it.
> >
> > It would be interesting to verify that cryptsetup cannot create a
> > LUKS1/LUKS2 volume where the data offset is not 512-byte aligned. I
> > think this is true, but the LUKS2 standard has the offset in bytes, so
> > its technically possible. If cryptsetup can create a volume where the
> > encrypted data is not 512-aligned, then there's a good case for adding
> > '-o'.
>
> cryptsetup supports only 512 byte block alignment for LUKS1 and 512-4096 alignment for LUKS2. Its offset parameter is specified in terms of the number of 512 byte blocks.
The LUKS1 payload offset is in 512-byte sectors. However, the LUKS2
segment offset is in _bytes_[1]. So my point still stands.
[1]
https://fossies.org/linux/cryptsetup/docs/on-disk-format-luks2.pdf#subsection.3.3
> > > Also, I am thinking whether it will be easier from the user perspective to support blocklist syntax (without
> > > loopback) for device argument too - having the same syntax for device and offset arguments is more clear.
> >
> >
> > Do you mean "file argument" instead of "device argument" here? Because
> > devices already support blocklist syntax.
> >
> > > However, it will works only if 'grub_file_t' provides interface to 'grub_disk_t' object which is needed for
> > > cryptodisk to read encrypted data. I didn't look at it, but if it works, the command syntax can be simplified
> > > to blocklist syntax for both device and offset argument.
> >
> >
> > I'm not sure I'm understanding this. Can you give some examples of what
> > the new command syntax that you're thinking of might look like? How
> > would blocklist syntax be used for the offset argument? That sounds
> > like it could be confusing.
> >
> > Glenn
> >
>
> I was not saying about some new syntax for some device offset argument.
> I am speaking about this:
>
> loopback node (hd0,gpt2)2048+
> plainmount node -c aes-xts-plain64 -h sha512 // works
> plainmount (hd0,gpt2)2048+ -c aes-xts-plain64 -h sha512 // currently - no
>
> Currently the second command does not work without loopback, I am thinking about removing this limitation in plainmount.
Ahh, yes, that does not work. Instead of removing this limitation for
plainmount, I think it would be better to remove this limitation for
all instances where devices are given to commands. Basically allow
grub_device_open to open blocklist syntax. Without much research, I
think this would entail adding an offset grub_disk_t and refactoring
grub_fs_blocklist_open() to use a common function that does the block
list parsing and that can then be reused in grub_disk_open(). I do not
really like the idea of having this as a special case for plainmount.
Glenn
>
> Best regards,
> Maxim Fomin
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-06-30 19:49 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-18 14:33 [PATCH v3 1/1] plainmount: Support plain encryption mode Maxim Fomin
2022-06-25 0:55 ` Glenn Washburn
2022-06-25 11:19 ` Maxim Fomin
2022-06-26 13:37 ` Maxim Fomin
2022-06-26 21:20 ` Glenn Washburn
2022-06-28 5:07 ` Maxim Fomin
2022-06-28 17:46 ` Glenn Washburn
2022-06-28 21:07 ` Maxim Fomin
2022-06-30 19:43 ` 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.