Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
On Wed, Dec 08, 2021 at 12:18:13PM -0600, Glenn Washburn wrote:
> On Wed, 8 Dec 2021 17:37:19 +0100
> Daniel Kiper wrote:
>
> > 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
> > > ---
> > > 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?
>
> I've re-defined success (that is return == GRUB_ERR_NONE) in
> grub_cryptodisk_scan_device_real to be such that, for the case where we
> are looking for a particular UUID, either source we're given matches
> that UUID is already opened or we successfully open it. If a UUID is
> not being searched for, then the criteria is essentially the same,
> except for the UUID check.
>
> When searching, GRUB_ERR_EXISTS is returned when the source is
> associated with an unlocked crypto device, but is not the UUID that is
> being searched for. This in turn tells grub_cryptodisk_scan_device to
> not return true (ie. do not stop searching for the requested UUID).
>
> Looking at this again, I'm thinking it might be clearer if
> grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
> either opens the source or source is associated with an already opened
> crypto device, and otehrwise return NULL. If the caller receives a
> non-NULL, and does the UUID check itself, if it cares/needs to.
>
> I'm also noticing that at a minimum, I think the if statement with the
> UUID check needs to be updated to "search_uuid == NULL ||
> grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
> like being sent a NULL pointer. Though, I'm confused why this didn't
> crash in my testing.
OK, please fix the patch and add a comment if you do some not obvious
things like here.
> > > +}
> > >
> > >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?
>
> If this return is reached, it means that all cryptodisk backend modules
> (eg. luks, luks2, geli) have tried unsuccessfully to open source. We
> need to return an error here does that grub_cryptodisk_scan_device does
> not confuse a success here with meaning that the source was
> successfully opened.
>
> This was not needed before because "have_it" was being used to
> determine if source was or has been successfully opened. With these
> changes it is not the return code of grub_cryptodisk_scan_device_real
> that is being used for this.
I think this begs for a comment too...
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
On Wed, 8 Dec 2021 17:37:19 +0100
Daniel Kiper wrote:
> 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
> > ---
> > 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?
I've re-defined success (that is return == GRUB_ERR_NONE) in
grub_cryptodisk_scan_device_real to be such that, for the case where we
are looking for a particular UUID, either source we're given matches
that UUID is already opened or we successfully open it. If a UUID is
not being searched for, then the criteria is essentially the same,
except for the UUID check.
When searching, GRUB_ERR_EXISTS is returned when the source is
associated with an unlocked crypto device, but is not the UUID that is
being searched for. This in turn tells grub_cryptodisk_scan_device to
not return true (ie. do not stop searching for the requested UUID).
Looking at this again, I'm thinking it might be clearer if
grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
either opens the source or source is associated with an already opened
crypto device, and otehrwise return NULL. If the caller receives a
non-NULL, and does the UUID check itself, if it cares/needs to.
I'm also noticing that at a minimum, I think the if statement with the
UUID check needs to be updated to "search_uuid == NULL ||
grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
like being sent a NULL pointer. Though, I'm confused why this didn't
crash in my testing.
> > +}
> >
> >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?
If this return is reached, it means that all cryptodisk backend modules
(eg. luks, luks2, geli) have tried unsuccessfully to open source. We
need to return an error here does that grub_cryptodisk_scan_device does
not confuse a success here with meaning that the source was
successfully opened.
This was not needed before because "have_it" was being used to
determine if source was or has been successfully opened. With these
changes it is not the return code of grub_cryptodisk_scan_device_real
that is being used for this.
> > }
> >
> > #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 &&
Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
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
> ---
> 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 (_cryptodisk_scan_device, NULL);
> + found_uuid = grub_device_iterate (_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
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
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
---
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;
+}
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");
}
#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 (_cryptodisk_scan_device, NULL);
+ found_uuid = grub_device_iterate (_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,
--
2.27.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
