Re: [PATCH v18 21/25] cryptodisk: Fallback to passphrase

[Gary Lin]

On Fri, Aug 30, 2024 at 06:10:28PM +0200, Daniel Kiper wrote:
> On Fri, Jun 28, 2024 at 04:19:04PM +0800, Gary Lin via Grub-devel wrote:
> > From: Patrick Colp <patrick.colp@oracle.com>
> >
> > If a protector is specified, but it fails to unlock the disk, fall back
> > to asking for the passphrase. However, an error was set indicating that
> > the protector(s) failed. Later code (e.g., LUKS code) fails as
> > `grub_errno` is now set. Print the existing errors out first, before
> > proceeding with the passphrase.
> 
> This behavior has to be documented in the GRUB docs.
> 
I'll address the behavior in the section for TPM2 key protector.

> > Signed-off-by: Patrick Colp <patrick.colp@oracle.com>
> > Signed-off-by: Gary Lin <glin@suse.com>
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 6f7394942..1a994d935 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1167,6 +1167,10 @@ grub_cryptodisk_scan_device_real (const char *name,
> >       ret = cr->recover_key (source, dev, cargs);
> >       if (ret != GRUB_ERR_NONE)
> >         {
> > +         /* Reset key data to trigger the passphrase prompt later */
> > +         cargs->key_data = NULL;
> > +         cargs->key_len = 0;
> > +
> >           part = grub_partition_get_name (source->partition);
> >           grub_dprintf ("cryptodisk",
> >                         "recovered a key from key protector %s but it "
> > @@ -1192,7 +1196,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >               source->name, source->partition != NULL ? "," : "",
> >               part != NULL ? part : N_("UNKNOWN"), dev->uuid);
> >        grub_free (part);
> > -      goto error;
> >      }
> >
> >    if (cargs->key_len)
> > @@ -1207,6 +1210,18 @@ grub_cryptodisk_scan_device_real (const char *name,
> >        unsigned long tries = 3;
> >        const char *tries_env;
> >
> > +      /*
> > +       * Print the error from key protectors and clear grub_errno.
> 
> I think you should explain why you have to do it here. Something similar
> to the commit message...
> 
It's mainly to print the error from the key protector. I'll add more
comments for that.

> > +       * Since '--protector' doesn't not coexist with '--password' and
> 
> s/doesn't not/cannot/?
> 
Will fix it in the next versino.

> > +       * '--key-file', only "cargs->key_len == 0" is expected if all
> > +       * key protectors fail.
> > +       */
> > +      if (grub_errno)
> 
> if (grub_errno != GRUB_ERR_NONE)
> 
Ok. I'll fix it.

Gary Lin

> > +   {
> > +     grub_print_error ();
> > +     grub_errno = GRUB_ERR_NONE;
> > +   }
> 
> Daniel