On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote: > The crypto device modules should only be setting up the crypto devices and > not getting user input. This has the added benefit of simplifying the code > such that three essentially duplicate pieces of code are merged into one.
Mostly nitpicks below... > Signed-off-by: Glenn Washburn <developm...@efficientek.com> > --- > grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++------- > grub-core/disk/geli.c | 26 ++++--------------- > grub-core/disk/luks.c | 27 +++---------------- > grub-core/disk/luks2.c | 26 ++++--------------- > include/grub/cryptodisk.h | 1 + > 5 files changed, 57 insertions(+), 75 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index 577942088..a5f7b860c 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name, > grub_disk_t source, > grub_cryptomount_args_t cargs) > { > - grub_err_t err; > + grub_err_t ret = GRUB_ERR_NONE; > grub_cryptodisk_t dev; > grub_cryptodisk_dev_t cr; > + int askpass = 0; > + char *part = NULL; > > dev = grub_cryptodisk_get_by_source_disk (source); > > @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name, > return grub_errno; > if (!dev) > continue; > - > - err = cr->recover_key (source, dev, cargs); > - if (err) > - { > - cryptodisk_close (dev); > - return err; > - } > + > + if (cargs->key_len == 0) I am OK with "if (!cargs->key_len)" for all kinds of numeric values... > + { > + /* Get the passphrase from the user, if no key data. */ > + askpass = 1; > + if (source->partition) ...but not for the pointers and enums. s/if (source->partition)/if (source->partition != NULL)/ > + part = grub_partition_get_name (source->partition); > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name, > + source->partition ? "," : "", part ? : "", s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/ s/part ? : ""/part != NULL ? part : "NO NAME"/? > + dev->uuid); > + grub_free (part); > + > + cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE); > + if (!cargs->key_data) Ditto and below please... > + return grub_errno; > + > + if (!grub_password_get ((char *) cargs->key_data, > GRUB_CRYPTODISK_MAX_PASSPHRASE)) > + { > + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); All errors printed by grub_error() should start with lower case... > + goto error; > + } > + cargs->key_len = grub_strlen ((char *) cargs->key_data); > + } > + > + ret = cr->recover_key (source, dev, cargs); > + if (ret) if (ret != GRUB_ERR_NONE) > + goto error; > > grub_cryptodisk_insert (dev, name, source); > > have_it = 1; > > - return GRUB_ERR_NONE; > + goto cleanup; > } > - return GRUB_ERR_NONE; > + goto cleanup; > + > +error: Please put space before labels. > + cryptodisk_close (dev); I would add empty line here. > +cleanup: Please put space before labels. > + if (askpass) > + { > + cargs->key_len = 0; > + grub_free (cargs->key_data); > + } > + return ret; > } > > #ifdef GRUB_UTIL > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index 4e8c377e7..32f34d5c3 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -135,8 +135,6 @@ const char *algorithms[] = { > [0x16] = "aes" > }; > > -#define MAX_PASSPHRASE 256 > - > static gcry_err_code_t > geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno) > { > @@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, > grub_cryptomount_args_t > grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN]; > grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE]; > grub_uint8_t geli_cipher_key[64]; > - char passphrase[MAX_PASSPHRASE] = ""; > unsigned i; > gcry_err_code_t gcry_err; > struct grub_geli_phdr header; > - char *tmp; > grub_disk_addr_t sector; > grub_err_t err; > > - /* Keyfiles are not implemented yet */ > - if (cargs->key_data || cargs->key_len) > - return GRUB_ERR_NOT_IMPLEMENTED_YET; > + if (cargs->key_data == NULL || cargs->key_len == 0) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data"); All errors printed by grub_error() should start with lower case... > if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE) > return grub_error (GRUB_ERR_BUG, "cipher block is too long"); > @@ -438,23 +433,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, > grub_cryptomount_args_t > > grub_puts_ (N_("Attempting to decrypt master key...")); > > - /* Get the passphrase from the user. */ > - tmp = NULL; > - if (source->partition) > - tmp = grub_partition_get_name (source->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name, > - source->partition ? "," : "", tmp ? : "", > - dev->uuid); > - grub_free (tmp); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); > - > /* Calculate the PBKDF2 of the user supplied passphrase. */ > if (grub_le_to_cpu32 (header.niter) != 0) > { > grub_uint8_t pbkdf_key[64]; > - gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase, > - grub_strlen (passphrase), > + gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data, > + cargs->key_len, > header.salt, > sizeof (header.salt), > grub_le_to_cpu32 (header.niter), > @@ -477,7 +461,7 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, > grub_cryptomount_args_t > return grub_crypto_gcry_error (GPG_ERR_OUT_OF_MEMORY); > > grub_crypto_hmac_write (hnd, header.salt, sizeof (header.salt)); > - grub_crypto_hmac_write (hnd, passphrase, grub_strlen (passphrase)); > + grub_crypto_hmac_write (hnd, cargs->key_data, cargs->key_len); > > gcry_err = grub_crypto_hmac_fini (hnd, geomkey); > if (gcry_err) > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 0462edc6e..51646cefe 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -29,8 +29,6 @@ > > GRUB_MOD_LICENSE ("GPLv3+"); > > -#define MAX_PASSPHRASE 256 > - > #define LUKS_KEY_ENABLED 0x00AC71F3 > > /* On disk LUKS header */ > @@ -158,17 +156,14 @@ luks_recover_key (grub_disk_t source, > struct grub_luks_phdr header; > grub_size_t keysize; > grub_uint8_t *split_key = NULL; > - char passphrase[MAX_PASSPHRASE] = ""; > grub_uint8_t candidate_digest[sizeof (header.mkDigest)]; > unsigned i; > grub_size_t length; > grub_err_t err; > grub_size_t max_stripes = 1; > - char *tmp; > > - /* Keyfiles are not implemented yet */ > - if (cargs->key_data || cargs->key_len) > - return GRUB_ERR_NOT_IMPLEMENTED_YET; > + if (cargs->key_data == NULL || cargs->key_len == 0) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data"); Same as above... > err = grub_disk_read (source, 0, 0, sizeof (header), &header); > if (err) > @@ -188,20 +183,6 @@ luks_recover_key (grub_disk_t source, > if (!split_key) > return grub_errno; > > - /* Get the passphrase from the user. */ > - tmp = NULL; > - if (source->partition) > - tmp = grub_partition_get_name (source->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name, > - source->partition ? "," : "", tmp ? : "", > - dev->uuid); > - grub_free (tmp); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > - { > - grub_free (split_key); > - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); > - } > - > /* Try to recover master key from each active keyslot. */ > for (i = 0; i < ARRAY_SIZE (header.keyblock); i++) > { > @@ -216,8 +197,8 @@ luks_recover_key (grub_disk_t source, > grub_dprintf ("luks", "Trying keyslot %d\n", i); > > /* Calculate the PBKDF2 of the user supplied passphrase. */ > - gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase, > - grub_strlen (passphrase), > + gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data, > + cargs->key_len, > header.keyblock[i].passwordSalt, > sizeof (header.keyblock[i].passwordSalt), > grub_be_to_cpu32 (header.keyblock[i]. > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > index 455a78cb0..c77380cbb 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -35,8 +35,6 @@ GRUB_MOD_LICENSE ("GPLv3+"); > #define LUKS_MAGIC_1ST "LUKS\xBA\xBE" > #define LUKS_MAGIC_2ND "SKUL\xBA\xBE" > > -#define MAX_PASSPHRASE 256 > - > enum grub_luks2_kdf_type > { > LUKS2_KDF_TYPE_ARGON2I, > @@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source, > grub_cryptomount_args_t cargs) > { > grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN]; > - char passphrase[MAX_PASSPHRASE], cipher[32]; > - char *json_header = NULL, *part = NULL, *ptr; > + char cipher[32]; If you change that could you add a comment why cipher length is equal 32? > + char *json_header = NULL, *ptr; > grub_size_t candidate_key_len = 0, json_idx, size; > grub_luks2_header_t header; > grub_luks2_keyslot_t keyslot; > @@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source, > grub_json_t *json = NULL, keyslots; > grub_err_t ret; > > - /* Keyfiles are not implemented yet */ > - if (cargs->key_data || cargs->key_len) > - return GRUB_ERR_NOT_IMPLEMENTED_YET; > + if (cargs->key_data == NULL || cargs->key_len == 0) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data"); Same as above... Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel