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

[Glenn Washburn]

On Fri, 3 Dec 2021 22:35:11 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Dec 03, 2021 at 03:04:36PM -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"/?
> >
> > For completeness, I missed a part of your suggestion in my original
> > response. I don't believe we should use "NO NAME" because a part ==
> > NULL means that the source device is not a partition (or that
> > grub_partition_get_name fails, which only happens if grub_malloc
> > fails). So the empty string is more appropriate. I could add an extra
> 
> ... but the problem with empty string is it is invisible for the user.
> That is why I do not like it.

That's the whole point though. For the majority of cases where part ==
NULL a whole disk is being attempted to be decrypted, so there is no
partition. In this case, you want the third %s to be empty. Only in the
extremely rare case that a partition is being attempted and we have a
grub_malloc failure in grub_partition_get_name does the empty string
for the third %s become undesirable.

Anyway, the next version will address this.

> > tertiary operator to select "NO NAME" (or better "UNKNOWN"?) if
> 
> I concur.
> 
> > source->partition != NULL. I think this should be done in a different
> > patch though.
> 
> If you wish go ahead.
> 
> Daniel

Glenn