Re: [PATCH v3 2/4] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk

[Daniel Kiper]

On Wed, Dec 01, 2021 at 03:48:40PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 20:10:21 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > 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 <development@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"/?
>
> Ok, when moving code, I generally don't like to change it unless
> necesary. I'll add these changes.

Yeah, I agree but I would make an exception here.

> > > +              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...
>
> Ok, I'll try to keep that in mind. There's quite a few grub_error()
> calls in cryptodisk that do not conform to that (and I expect else
> where in the source).

Yeah, it would be nice to fix it. If you do not mind please make a patch
and I will take it.

> > > +     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.
>
> Are you saying you want the line to be " error:"? There are labels in
> the source which are preceeded by whitespace, but they seem to be in
> the minority. What's the rationale for this? or is it just personal
> preference? I don't mind making this change, but I don't understand it.

It differentiates a bit labels from e.g. functions names which starts in
the first column. I got used to it when I was working on the Linux
kernel and Xen. I can agree this is not perfect but...

[...]

> > > 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?
>
> I'm not sure why. I think that's a question for Patrick. I'd guess he
> figured it was a resonable upper bound on the length of the cipher
> string.

Patrick, could you chime in?

Daniel