[PATCH v4] cryptodisk: allow user to retry failed passphrase

[PATCH v4] cryptodisk: allow user to retry failed passphrase

[Forest]

Give the user a chance to re-enter their cryptodisk passphrase after a typo,
rather than immediately failing (and likely dumping them into a grub shell).

By default, we allow 3 tries before giving up. A value in the
cryptodisk_passphrase_tries environment variable will override this default.

The user can give up early by entering an empty passphrase, just as they
could before this patch.

Signed-off-by: Forest <forestix@nom.one>
---
Note: This is identical to the v3 patch. The change I had in mind proved unviable.

 docs/grub.texi              |  9 +++++
 grub-core/disk/cryptodisk.c | 74 +++++++++++++++++++++++++++++--------
 2 files changed, 67 insertions(+), 16 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index a225f9a88..6ac603a32 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3277,6 +3277,7 @@ These variables have special meaning to GRUB.
 * color_normal::
 * config_directory::
 * config_file::
+* cryptodisk_passphrase_tries::
 * debug::
 * default::
 * fallback::
@@ -3441,6 +3442,14 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
 (@pxref{normal}).  It is restored to the previous value when command completes.
 
 
+@node cryptodisk_passphrase_tries
+@subsection cryptodisk_passphrase_tries
+
+When prompting the user for a cryptodisk passphrase, allow this many attempts
+before giving up. The default is @samp{3}. (The user can give up early by
+entering an empty passphrase.)
+
+
 @node debug
 @subsection debug
 
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 2246af51b..4fa7dc58d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -17,6 +17,7 @@
  */
 
 #include <grub/cryptodisk.h>
+#include <grub/env.h>
 #include <grub/mm.h>
 #include <grub/misc.h>
 #include <grub/dl.h>
@@ -1063,6 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
   grub_cryptodisk_dev_t cr;
   struct cryptodisk_read_hook_ctx read_hook_data = {0};
   int askpass = 0;
+  unsigned long tries = 3;
+  const char *tries_env;
   char *part = NULL;
 
   dev = grub_cryptodisk_get_by_source_disk (source);
@@ -1114,32 +1117,71 @@ grub_cryptodisk_scan_device_real (const char *name,
     if (!dev)
       continue;
 
-    if (!cargs->key_len)
+    if (cargs->key_len)
+      {
+	ret = cr->recover_key (source, dev, cargs);
+	if (ret != GRUB_ERR_NONE)
+	  goto error;
+      }
+    else
       {
 	/* Get the passphrase from the user, if no key data. */
 	askpass = 1;
-	part = grub_partition_get_name (source->partition);
-	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-		     source->partition != NULL ? "," : "",
-		     part != NULL ? part : N_("UNKNOWN"),
-		     dev->uuid);
-	grub_free (part);
-
 	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
 	if (cargs->key_data == NULL)
 	  goto error_no_close;
 
-	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+	tries_env = grub_env_get ("cryptodisk_passphrase_tries");
+	if (tries_env != NULL && tries_env[0] != '\0')
 	  {
-	    grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
-	    goto error;
+	    const char *p = NULL;
+	    tries = grub_strtoul (tries_env, &p, 0);
+	    if (*p != '\0')
+	      {
+		/* Account for grub_strtoul() ignoring trailing text. */
+		grub_err_t err = grub_errno;
+		if (p > tries_env && tries != ~0UL)
+		  err = GRUB_ERR_BAD_NUMBER;
+
+		grub_error (err,
+			    N_("non-numeric or invalid value for cryptodisk_passphrase_tries: `%s'"),
+			    tries_env);
+		goto error;
+	      }
 	  }
-	cargs->key_len = grub_strlen ((char *) cargs->key_data);
-      }
 
-    ret = cr->recover_key (source, dev, cargs);
-    if (ret != GRUB_ERR_NONE)
-      goto error;
+	for (; tries > 0; tries--)
+	  {
+	    part = grub_partition_get_name (source->partition);
+	    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+			 source->partition != NULL ? "," : "",
+			 part != NULL ? part : N_("UNKNOWN"),
+			 dev->uuid);
+	    grub_free (part);
+
+	    if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+	      {
+		grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
+		goto error;
+	      }
+	    cargs->key_len = grub_strlen ((char *) cargs->key_data);
+
+	    ret = cr->recover_key (source, dev, cargs);
+	    if (ret == GRUB_ERR_NONE)
+	      break;
+	    if (ret != GRUB_ERR_ACCESS_DENIED || tries == 1)
+	      goto error;
+	    grub_puts_ (N_("Invalid passphrase."));
+
+	    /*
+	     * Since recover_key() calls a function that returns grub_errno,
+	     * a leftover error value from a previously rejected passphrase
+	     * will trigger a phantom failure. We therefore clear it before
+	     * trying a new passphrase.
+	     */
+	    grub_errno = GRUB_ERR_NONE;
+	  }
+      }
 
     ret = grub_cryptodisk_insert (dev, name, source);
     if (ret != GRUB_ERR_NONE)
-- 
2.39.2

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4] cryptodisk: allow user to retry failed passphrase

[Glenn Washburn]

On Sat, 27 Apr 2024 17:48:31 -0700
Forest <forestix@nom.one> wrote:

> Give the user a chance to re-enter their cryptodisk passphrase after a typo,
> rather than immediately failing (and likely dumping them into a grub shell).
> 
> By default, we allow 3 tries before giving up. A value in the
> cryptodisk_passphrase_tries environment variable will override this default.
> 
> The user can give up early by entering an empty passphrase, just as they
> could before this patch.
> 
> Signed-off-by: Forest <forestix@nom.one>
> ---
> Note: This is identical to the v3 patch. The change I had in mind proved unviable.

LGTM.

Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

> 
>  docs/grub.texi              |  9 +++++
>  grub-core/disk/cryptodisk.c | 74 +++++++++++++++++++++++++++++--------
>  2 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index a225f9a88..6ac603a32 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3277,6 +3277,7 @@ These variables have special meaning to GRUB.
>  * color_normal::
>  * config_directory::
>  * config_file::
> +* cryptodisk_passphrase_tries::
>  * debug::
>  * default::
>  * fallback::
> @@ -3441,6 +3442,14 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
>  (@pxref{normal}).  It is restored to the previous value when command completes.
>  
>  
> +@node cryptodisk_passphrase_tries
> +@subsection cryptodisk_passphrase_tries
> +
> +When prompting the user for a cryptodisk passphrase, allow this many attempts
> +before giving up. The default is @samp{3}. (The user can give up early by
> +entering an empty passphrase.)
> +
> +
>  @node debug
>  @subsection debug
>  
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 2246af51b..4fa7dc58d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #include <grub/cryptodisk.h>
> +#include <grub/env.h>
>  #include <grub/mm.h>
>  #include <grub/misc.h>
>  #include <grub/dl.h>
> @@ -1063,6 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
>    grub_cryptodisk_dev_t cr;
>    struct cryptodisk_read_hook_ctx read_hook_data = {0};
>    int askpass = 0;
> +  unsigned long tries = 3;
> +  const char *tries_env;
>    char *part = NULL;
>  
>    dev = grub_cryptodisk_get_by_source_disk (source);
> @@ -1114,32 +1117,71 @@ grub_cryptodisk_scan_device_real (const char *name,
>      if (!dev)
>        continue;
>  
> -    if (!cargs->key_len)
> +    if (cargs->key_len)
> +      {
> +	ret = cr->recover_key (source, dev, cargs);
> +	if (ret != GRUB_ERR_NONE)
> +	  goto error;
> +      }
> +    else
>        {
>  	/* Get the passphrase from the user, if no key data. */
>  	askpass = 1;
> -	part = grub_partition_get_name (source->partition);
> -	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -		     source->partition != NULL ? "," : "",
> -		     part != NULL ? part : N_("UNKNOWN"),
> -		     dev->uuid);
> -	grub_free (part);
> -
>  	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
>  	if (cargs->key_data == NULL)
>  	  goto error_no_close;
>  
> -	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +	tries_env = grub_env_get ("cryptodisk_passphrase_tries");
> +	if (tries_env != NULL && tries_env[0] != '\0')
>  	  {
> -	    grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> -	    goto error;
> +	    const char *p = NULL;
> +	    tries = grub_strtoul (tries_env, &p, 0);
> +	    if (*p != '\0')
> +	      {
> +		/* Account for grub_strtoul() ignoring trailing text. */
> +		grub_err_t err = grub_errno;
> +		if (p > tries_env && tries != ~0UL)
> +		  err = GRUB_ERR_BAD_NUMBER;
> +
> +		grub_error (err,
> +			    N_("non-numeric or invalid value for cryptodisk_passphrase_tries: `%s'"),
> +			    tries_env);
> +		goto error;
> +	      }
>  	  }
> -	cargs->key_len = grub_strlen ((char *) cargs->key_data);
> -      }
>  
> -    ret = cr->recover_key (source, dev, cargs);
> -    if (ret != GRUB_ERR_NONE)
> -      goto error;
> +	for (; tries > 0; tries--)
> +	  {
> +	    part = grub_partition_get_name (source->partition);
> +	    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +			 source->partition != NULL ? "," : "",
> +			 part != NULL ? part : N_("UNKNOWN"),
> +			 dev->uuid);
> +	    grub_free (part);
> +
> +	    if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +	      {
> +		grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> +		goto error;
> +	      }
> +	    cargs->key_len = grub_strlen ((char *) cargs->key_data);
> +
> +	    ret = cr->recover_key (source, dev, cargs);
> +	    if (ret == GRUB_ERR_NONE)
> +	      break;
> +	    if (ret != GRUB_ERR_ACCESS_DENIED || tries == 1)
> +	      goto error;
> +	    grub_puts_ (N_("Invalid passphrase."));
> +
> +	    /*
> +	     * Since recover_key() calls a function that returns grub_errno,
> +	     * a leftover error value from a previously rejected passphrase
> +	     * will trigger a phantom failure. We therefore clear it before
> +	     * trying a new passphrase.
> +	     */
> +	    grub_errno = GRUB_ERR_NONE;
> +	  }
> +      }
>  
>      ret = grub_cryptodisk_insert (dev, name, source);
>      if (ret != GRUB_ERR_NONE)

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4] cryptodisk: allow user to retry failed passphrase

[Maximilian Stendler]

Hi,

I've had the same issue but build a different solution via scripting[0]:
The cryptomount and retries are implemented in a grub.cfg template. With
some shell-scripts the actual grub.cfg can be generated and put into a
tar used as a memdisk, additionally containing all necessary modules and
compressed.
The custom efi to be used in the esp is generated with `grub-mkimage`
and an initial.cfg, which decompresses and mounts the tar with the
modules and sources the actual grub.cfg.

The behavior  differs a bit from this solution: It has unlimited retries
(need to CTRL+ALT+DEL to reboot) and thus never enters a rescue shell,
it has a (configurable) 3-second timeout between tries and prints a
red-colored (but not localized) message on a wrong password.


With that being said, I am definitely all for having such behavior in
the bootx64.efi generated by grub-mkinstall with disk encryption
enabled. That is probably more likely to be picked up by distros and
reaching more end users than my badly named repo with a hacked solution. :')

Cheers
Max

[0] https://github.com/stendler/grub-fde
The repo also contains scripts to test such generated efi files with
qemu and a Containerfile for a grub build environment in a podman/docker
image to test generating with different grub versions.

On 2024-04-28 02:48, Forest wrote:
> Give the user a chance to re-enter their cryptodisk passphrase after a typo,
> rather than immediately failing (and likely dumping them into a grub shell).
> 
> By default, we allow 3 tries before giving up. A value in the
> cryptodisk_passphrase_tries environment variable will override this default.
> 
> The user can give up early by entering an empty passphrase, just as they
> could before this patch.
> 
> Signed-off-by: Forest <forestix@nom.one>
> ---
> Note: This is identical to the v3 patch. The change I had in mind proved unviable.
> 
>  docs/grub.texi              |  9 +++++
>  grub-core/disk/cryptodisk.c | 74 +++++++++++++++++++++++++++++--------
>  2 files changed, 67 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index a225f9a88..6ac603a32 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3277,6 +3277,7 @@ These variables have special meaning to GRUB.
>  * color_normal::
>  * config_directory::
>  * config_file::
> +* cryptodisk_passphrase_tries::
>  * debug::
>  * default::
>  * fallback::
> @@ -3441,6 +3442,14 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
>  (@pxref{normal}).  It is restored to the previous value when command completes.
>  
>  
> +@node cryptodisk_passphrase_tries
> +@subsection cryptodisk_passphrase_tries
> +
> +When prompting the user for a cryptodisk passphrase, allow this many attempts
> +before giving up. The default is @samp{3}. (The user can give up early by
> +entering an empty passphrase.)
> +
> +
>  @node debug
>  @subsection debug
>  
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 2246af51b..4fa7dc58d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -17,6 +17,7 @@
>   */
>  
>  #include <grub/cryptodisk.h>
> +#include <grub/env.h>
>  #include <grub/mm.h>
>  #include <grub/misc.h>
>  #include <grub/dl.h>
> @@ -1063,6 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
>    grub_cryptodisk_dev_t cr;
>    struct cryptodisk_read_hook_ctx read_hook_data = {0};
>    int askpass = 0;
> +  unsigned long tries = 3;
> +  const char *tries_env;
>    char *part = NULL;
>  
>    dev = grub_cryptodisk_get_by_source_disk (source);
> @@ -1114,32 +1117,71 @@ grub_cryptodisk_scan_device_real (const char *name,
>      if (!dev)
>        continue;
>  
> -    if (!cargs->key_len)
> +    if (cargs->key_len)
> +      {
> +	ret = cr->recover_key (source, dev, cargs);
> +	if (ret != GRUB_ERR_NONE)
> +	  goto error;
> +      }
> +    else
>        {
>  	/* Get the passphrase from the user, if no key data. */
>  	askpass = 1;
> -	part = grub_partition_get_name (source->partition);
> -	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -		     source->partition != NULL ? "," : "",
> -		     part != NULL ? part : N_("UNKNOWN"),
> -		     dev->uuid);
> -	grub_free (part);
> -
>  	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
>  	if (cargs->key_data == NULL)
>  	  goto error_no_close;
>  
> -	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +	tries_env = grub_env_get ("cryptodisk_passphrase_tries");
> +	if (tries_env != NULL && tries_env[0] != '\0')
>  	  {
> -	    grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> -	    goto error;
> +	    const char *p = NULL;
> +	    tries = grub_strtoul (tries_env, &p, 0);
> +	    if (*p != '\0')
> +	      {
> +		/* Account for grub_strtoul() ignoring trailing text. */
> +		grub_err_t err = grub_errno;
> +		if (p > tries_env && tries != ~0UL)
> +		  err = GRUB_ERR_BAD_NUMBER;
> +
> +		grub_error (err,
> +			    N_("non-numeric or invalid value for cryptodisk_passphrase_tries: `%s'"),
> +			    tries_env);
> +		goto error;
> +	      }
>  	  }
> -	cargs->key_len = grub_strlen ((char *) cargs->key_data);
> -      }
>  
> -    ret = cr->recover_key (source, dev, cargs);
> -    if (ret != GRUB_ERR_NONE)
> -      goto error;
> +	for (; tries > 0; tries--)
> +	  {
> +	    part = grub_partition_get_name (source->partition);
> +	    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +			 source->partition != NULL ? "," : "",
> +			 part != NULL ? part : N_("UNKNOWN"),
> +			 dev->uuid);
> +	    grub_free (part);
> +
> +	    if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +	      {
> +		grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> +		goto error;
> +	      }
> +	    cargs->key_len = grub_strlen ((char *) cargs->key_data);
> +
> +	    ret = cr->recover_key (source, dev, cargs);
> +	    if (ret == GRUB_ERR_NONE)
> +	      break;
> +	    if (ret != GRUB_ERR_ACCESS_DENIED || tries == 1)
> +	      goto error;
> +	    grub_puts_ (N_("Invalid passphrase."));
> +
> +	    /*
> +	     * Since recover_key() calls a function that returns grub_errno,
> +	     * a leftover error value from a previously rejected passphrase
> +	     * will trigger a phantom failure. We therefore clear it before
> +	     * trying a new passphrase.
> +	     */
> +	    grub_errno = GRUB_ERR_NONE;
> +	  }
> +      }
>  
>      ret = grub_cryptodisk_insert (dev, name, source);
>      if (ret != GRUB_ERR_NONE)

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4] cryptodisk: allow user to retry failed passphrase

[Daniel Kiper via Grub-devel]

On Sat, Apr 27, 2024 at 05:48:31PM -0700, Forest wrote:
> Give the user a chance to re-enter their cryptodisk passphrase after a typo,
> rather than immediately failing (and likely dumping them into a grub shell).
>
> By default, we allow 3 tries before giving up. A value in the
> cryptodisk_passphrase_tries environment variable will override this default.
>
> The user can give up early by entering an empty passphrase, just as they
> could before this patch.
>
> Signed-off-by: Forest <forestix@nom.one>
> ---
> Note: This is identical to the v3 patch. The change I had in mind proved unviable.
>
>  docs/grub.texi              |  9 +++++
>  grub-core/disk/cryptodisk.c | 74 +++++++++++++++++++++++++++++--------
>  2 files changed, 67 insertions(+), 16 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index a225f9a88..6ac603a32 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3277,6 +3277,7 @@ These variables have special meaning to GRUB.
>  * color_normal::
>  * config_directory::
>  * config_file::
> +* cryptodisk_passphrase_tries::
>  * debug::
>  * default::
>  * fallback::
> @@ -3441,6 +3442,14 @@ processed by commands @command{configfile} (@pxref{configfile}) or @command{norm
>  (@pxref{normal}).  It is restored to the previous value when command completes.
>
>
> +@node cryptodisk_passphrase_tries
> +@subsection cryptodisk_passphrase_tries
> +
> +When prompting the user for a cryptodisk passphrase, allow this many attempts
> +before giving up. The default is @samp{3}. (The user can give up early by
> +entering an empty passphrase.)
> +
> +
>  @node debug
>  @subsection debug
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 2246af51b..4fa7dc58d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -17,6 +17,7 @@
>   */
>
>  #include <grub/cryptodisk.h>
> +#include <grub/env.h>
>  #include <grub/mm.h>
>  #include <grub/misc.h>
>  #include <grub/dl.h>
> @@ -1063,6 +1064,8 @@ grub_cryptodisk_scan_device_real (const char *name,
>    grub_cryptodisk_dev_t cr;
>    struct cryptodisk_read_hook_ctx read_hook_data = {0};
>    int askpass = 0;
> +  unsigned long tries = 3;
> +  const char *tries_env;
>    char *part = NULL;
>
>    dev = grub_cryptodisk_get_by_source_disk (source);
> @@ -1114,32 +1117,71 @@ grub_cryptodisk_scan_device_real (const char *name,
>      if (!dev)
>        continue;
>
> -    if (!cargs->key_len)
> +    if (cargs->key_len)
> +      {
> +	ret = cr->recover_key (source, dev, cargs);
> +	if (ret != GRUB_ERR_NONE)
> +	  goto error;
> +      }
> +    else
>        {
>  	/* Get the passphrase from the user, if no key data. */
>  	askpass = 1;
> -	part = grub_partition_get_name (source->partition);
> -	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> -		     source->partition != NULL ? "," : "",
> -		     part != NULL ? part : N_("UNKNOWN"),
> -		     dev->uuid);
> -	grub_free (part);
> -
>  	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
>  	if (cargs->key_data == NULL)
>  	  goto error_no_close;
>
> -	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
> +	tries_env = grub_env_get ("cryptodisk_passphrase_tries");
> +	if (tries_env != NULL && tries_env[0] != '\0')
>  	  {
> -	    grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
> -	    goto error;
> +	    const char *p = NULL;
> +	    tries = grub_strtoul (tries_env, &p, 0);
> +	    if (*p != '\0')
> +	      {
> +		/* Account for grub_strtoul() ignoring trailing text. */
> +		grub_err_t err = grub_errno;
> +		if (p > tries_env && tries != ~0UL)
> +		  err = GRUB_ERR_BAD_NUMBER;

You can make this check much simpler. Please take a look at the commit
ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).

> +		grub_error (err,
> +			    N_("non-numeric or invalid value for cryptodisk_passphrase_tries: `%s'"),
> +			    tries_env);
> +		goto error;

In case of an error here I would assume default value. I think this
behavior should be documented then.

Otherwise patch LGTM...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4] cryptodisk: allow user to retry failed passphrase

[Forest]

On Mon, 6 May 2024 20:22:48 +0200, Daniel Kiper wrote:

>You can make this check much simpler. Please take a look at the commit
>ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).

I did it that way in order to preserve a feature of Glenn's suggested
approach: passing through any grub_errno value set by grub_strtoul().
(I merely added an additional case to update grub_errno if and only if we
generate an error of our own.)

Now that you mention it, though, I suppose it's okay to overwrite
grub_strtoul's errno values as long as the replacement values are sensible.
That function's logic seems unlikely to change in the future, after all, and
we're validating the string more strictly than it does.

>In case of an error here I would assume default value. I think this
>behavior should be documented then.

Yes, that makes sense, and would simplify the code a bit.
I'll do that in the next rev.

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel