Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global

[Daniel Kiper]

On Sat, Dec 04, 2021 at 01:15:45AM -0600, Glenn Washburn wrote:
> The global "have_it" was never used by the crypto-backends, but was used to
> determine if a crypto-backend successfully mounted a cryptodisk with a given
> uuid. This is not needed however, because grub_device_iterate() will return
> 1 if and only if grub_cryptodisk_scan_device() returns 1. And
> grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
> been specified and a cryptodisk was successfully setup by a crypto-backend.
>
> With this change grub_device_iterate() will return 1 when a crypto device is
> successfully decrypted or when the source device has already been
> successfully opened. Prior to this change, trying to mount an already
> successfully opened device would trigger an error with the message "no such
> cryptodisk found", which is at best misleading. The mount should silently
> succeed in this case, which is what happens with this patch.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 29 ++++++++++++++++++-----------
>  include/grub/err.h          |  1 +
>  2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..9224105ac 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
>  #endif
>
> -static int check_boot, have_it;
> +static int check_boot;
>  static char *search_uuid;
>
>  static void
> @@ -1005,7 +1005,12 @@ grub_cryptodisk_scan_device_real (const char *name, 
> grub_disk_t source)
>    dev = grub_cryptodisk_get_by_source_disk (source);
>
>    if (dev)
> -    return GRUB_ERR_NONE;
> +    {
> +      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> +     return GRUB_ERR_NONE;
> +      else
> +     return GRUB_ERR_EXISTS;

Hmmm... This looks strange. Could you explain why you return
GRUB_ERR_EXISTS if UUIDs do not match?

> +    }
>
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, 
> grub_disk_t source)
>
>      grub_cryptodisk_insert (dev, name, source);
>
> -    have_it = 1;
> -
>      return GRUB_ERR_NONE;
>    }
> -  return GRUB_ERR_NONE;
> +  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle 
> this device");

Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?

>  }
>
>  #ifdef GRUB_UTIL
> @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
>    err = grub_cryptodisk_scan_device_real (name, source);
>
>    grub_disk_close (source);
> -
> -  if (err)
> +
> +  /*
> +   * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many 
> unhelpful
> +   * error messages.
> +   */
> +  if (err != GRUB_ERR_NONE && err != GRUB_ERR_EXISTS && err != 
> GRUB_ERR_BAD_MODULE)
>      grub_print_error ();
> -  return have_it && search_uuid ? 1 : 0;
> +  return (err == GRUB_ERR_NONE && search_uuid != NULL);
>  }
>
>  static grub_err_t
> @@ -1110,9 +1117,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>    if (argc < 1 && !state[1].set && !state[2].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> -  have_it = 0;
>    if (state[0].set)
>      {
> +      int found_uuid;
>        grub_cryptodisk_t dev;
>
>        dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1125,10 +1132,10 @@ 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);
> +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
>        search_uuid = NULL;
>
> -      if (!have_it)
> +      if (!found_uuid)
>       return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>        return GRUB_ERR_NONE;
>      }
> diff --git a/include/grub/err.h b/include/grub/err.h
> index b08d5d0de..55219cdcc 100644
> --- a/include/grub/err.h
> +++ b/include/grub/err.h
> @@ -57,6 +57,7 @@ typedef enum
>      GRUB_ERR_MENU,
>      GRUB_ERR_TIMEOUT,
>      GRUB_ERR_IO,
> +    GRUB_ERR_EXISTS,
>      GRUB_ERR_ACCESS_DENIED,
>      GRUB_ERR_EXTRACTOR,
>      GRUB_ERR_NET_BAD_ADDRESS,

Daniel