grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 1/4] cryptodisk: Add infrastructure to pass data from cryp


From: Glenn Washburn
Subject: Re: [PATCH v3 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
Date: Wed, 1 Dec 2021 15:18:06 -0600

On Wed, 17 Nov 2021 18:29:36 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> > As an example, passing a password as a cryptomount argument is implemented.
> 
> I am not very happy with that. Splitting this into separate patch or
> merging with patch #2 probably would not help either.

Its not clear to me what action you're desiring. I don't particularly
like it either, but haven't thought of something better. Ideas?

> > However, the backends are not implemented, so testing this will return a not
> > implemented error.
> 
> The commit message lacks of explanation why this change is needed.
> I think you can copy part of the cover letter here.

I can add an explanation.

> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
> >  grub-core/disk/geli.c       |  6 +++++-
> >  grub-core/disk/luks.c       |  7 ++++++-
> >  grub-core/disk/luks2.c      |  7 ++++++-
> >  include/grub/cryptodisk.h   |  9 ++++++++-
> >  5 files changed, 46 insertions(+), 14 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..577942088 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> >      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
> >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> > +    {"password", 'p', 0, N_("Password to open volumes."), 0, 
> > ARG_TYPE_STRING},
> 
> Should not you update docs/grub.texi as well?

Yep, good catch. I think the doc change should be in patch #2 because
that's where the option actually becomes useful. What do you think?

> >      {0, 0, 0, 0, 0, 0}
> >    };
> >
> > @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> >  }
> >
> >  static grub_err_t
> > -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> > +grub_cryptodisk_scan_device_real (const char *name,
> > +                             grub_disk_t source,
> > +                             grub_cryptomount_args_t cargs)
> >  {
> >    grub_err_t err;
> >    grub_cryptodisk_t dev;
> > @@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name, 
> > grub_disk_t source)
> >      if (!dev)
> >        continue;
> >
> > -    err = cr->recover_key (source, dev);
> > +    err = cr->recover_key (source, dev, cargs);
> >      if (err)
> >      {
> >        cryptodisk_close (dev);
> > @@ -1080,10 +1083,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, 
> > const char *cheat)
> >
> >  static int
> >  grub_cryptodisk_scan_device (const char *name,
> > -                        void *data __attribute__ ((unused)))
> > +                        void *data)
> >  {
> >    grub_err_t err;
> >    grub_disk_t source;
> > +  grub_cryptomount_args_t cargs = data;
> >
> >    /* Try to open disk.  */
> >    source = grub_disk_open (name);
> > @@ -1093,7 +1097,7 @@ grub_cryptodisk_scan_device (const char *name,
> >        return 0;
> >      }
> >
> > -  err = grub_cryptodisk_scan_device_real (name, source);
> > +  err = grub_cryptodisk_scan_device_real (name, source, cargs);
> >
> >    grub_disk_close (source);
> >
> > @@ -1106,12 +1110,19 @@ static grub_err_t
> >  grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >  {
> >    struct grub_arg_list *state = ctxt->state;
> > +  struct grub_cryptomount_args cargs = {0};
> >
> >    if (argc < 1 && !state[1].set && !state[2].set)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
> >
> > +  if (state[3].set) /* password */
> > +    {
> > +      cargs.key_data = (grub_uint8_t *) state[3].arg;
> > +      cargs.key_len = grub_strlen (state[3].arg);
> > +    }
> > +
> >    have_it = 0;
> > -  if (state[0].set)
> > +  if (state[0].set) /* uuid */
> 
> Nice but if you want to do that please put that change into separate patch.
> Or at least advise in the commit message you are doing this on occasion.

I'll add a sentence to the commit message.

> >      {
> >        grub_cryptodisk_t dev;
> >
> > @@ -1125,18 +1136,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >
> >        check_boot = state[2].set;
> >        search_uuid = args[0];
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> > +      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> >        search_uuid = NULL;
> >
> >        if (!have_it)
> >     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >        return GRUB_ERR_NONE;
> >      }
> > -  else if (state[1].set || (argc == 0 && state[2].set))
> > +  else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> 
> Ditto.
> 
> >      {
> >        search_uuid = NULL;
> >        check_boot = state[2].set;
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> > +      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> >        search_uuid = NULL;
> >        return GRUB_ERR_NONE;
> >      }
> > @@ -1178,7 +1189,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >       return GRUB_ERR_NONE;
> >     }
> >
> > -      err = grub_cryptodisk_scan_device_real (diskname, disk);
> > +      err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
> >
> >        grub_disk_close (disk);
> >        if (disklast)
> > @@ -1317,7 +1328,7 @@ GRUB_MOD_INIT (cryptodisk)
> >  {
> >    grub_disk_dev_register (&grub_cryptodisk_dev);
> >    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> > -                         N_("SOURCE|-u UUID|-a|-b"),
> > +                         N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
> 
> s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?

The change you're suggesting indicates that "cryptomount -u UUID -p
password" is not correct usage of the command, only "cryptomount -p
password". My version doesn't support that either, but it does indicate
that "cryptomount -p password -u UUID" is an option. The idea behind my
version is that "-p password" is optional and can be used with any of
the other options, but not alone. So I don't believe the suggestion is
correct.

> >                           N_("Mount a crypto device."), options);
> >    grub_procfs_register ("luks_script", &luks_script);
> >  }
> > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > index 2f34a35e6..4e8c377e7 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char 
> > *check_uuid,
> >  }
> >
> >  static grub_err_t
> > -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> > +recover_key (grub_disk_t source, grub_cryptodisk_t dev, 
> > grub_cryptomount_args_t cargs)
> >  {
> >    grub_size_t keysize;
> >    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
> > @@ -414,6 +414,10 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> >    grub_disk_addr_t sector;
> >    grub_err_t err;
> >
> > +  /* Keyfiles are not implemented yet */
> > +  if (cargs->key_data || cargs->key_len)
> 
> if (cargs->key_data != NULL || cargs->key_len)
> 
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
> >      return grub_error (GRUB_ERR_BUG, "cipher block is too long");
> >
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 13103ea6a..0462edc6e 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char 
> > *check_uuid,
> >
> >  static grub_err_t
> >  luks_recover_key (grub_disk_t source,
> > -             grub_cryptodisk_t dev)
> > +             grub_cryptodisk_t dev,
> > +             grub_cryptomount_args_t cargs)
> >  {
> >    struct grub_luks_phdr header;
> >    grub_size_t keysize;
> > @@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
> >    grub_size_t max_stripes = 1;
> >    char *tmp;
> >
> > +  /* Keyfiles are not implemented yet */
> > +  if (cargs->key_data || cargs->key_len)
> 
> Ditto.
> 
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> >      return err;
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 371a53b83..455a78cb0 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >
> >  static grub_err_t
> >  luks2_recover_key (grub_disk_t source,
> > -              grub_cryptodisk_t crypt)
> > +              grub_cryptodisk_t crypt,
> > +              grub_cryptomount_args_t cargs)
> >  {
> >    grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
> >    char passphrase[MAX_PASSPHRASE], cipher[32];
> > @@ -556,6 +557,10 @@ 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)
> 
> Ditto.
> 
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    ret = luks2_read_header (source, &header);
> >    if (ret)
> >      return ret;
> 
> Daniel

Glenn




reply via email to

[Prev in Thread] Current Thread [Next in Thread]