Re: [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk

[Patrick Steinhardt]

On Mon, Sep 27, 2021 at 06:14:01PM -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.
> 
> 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 ca034859e..86eaabe60 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)
> +      {
> +     /* Get the passphrase from the user, if no key data. */
> +     askpass = 1;
> +     if (source->partition)
> +       part = grub_partition_get_name (source->partition);
> +     grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +                  source->partition ? "," : "", part ? : "",
> +                  dev->uuid);
> +     grub_free (part);
> +
> +     cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +     if (!cargs->key_data)
> +       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");
> +         goto error;
> +       }
> +     cargs->key_len = grub_strlen((char *) cargs->key_data);

Missing space.

> +      }
> +
> +    ret = cr->recover_key (source, dev, cargs);
> +    if (ret)
> +      goto error;
>  
>      grub_cryptodisk_insert (dev, name, source);
>  
>      have_it = 1;
>  
> -    return GRUB_ERR_NONE;
> +    goto cleanup;
>    }
> -  return GRUB_ERR_NONE;
> +  goto cleanup;
> +
> +error:
> +  cryptodisk_close (dev);
> +cleanup:
> +  if (askpass)
> +    {
> +      cargs->key_len = 0;
> +      grub_free(cargs->key_data);

Here, too. Otherwise, the patch looks good to me.

Patrick

signature.asc
Description: PGP signature