* [PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-16 18:39 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global Glenn Washburn
` (8 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/luks2.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..fea196dd4 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -370,7 +370,10 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
uuid[j] = '\0';
if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
- return NULL;
+ {
+ grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
+ return NULL;
+ }
cryptodisk = grub_zalloc (sizeof (*cryptodisk));
if (!cryptodisk)
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
2021-12-09 17:14 ` [PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-16 19:11 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded Glenn Washburn
` (7 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
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 or
a cryptodisk of the requested uuid is already open.
To implement this grub_cryptodisk_scan_device_real is modified to return a
cryptodisk or NULL on failure and having the appropriate grub_errno set to
indicated failure. Note that grub_cryptodisk_scan_device_real will fail now
with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
modules succeed in identifying the source disk.
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 | 56 +++++++++++++++++++++++--------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..9df3d310f 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
@@ -995,7 +995,7 @@ cryptodisk_close (grub_cryptodisk_t dev)
grub_free (dev);
}
-static grub_err_t
+static grub_cryptodisk_t
grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
{
grub_err_t err;
@@ -1005,13 +1005,13 @@ 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;
+ return dev;
FOR_CRYPTODISK_DEVS (cr)
{
dev = cr->scan (source, search_uuid, check_boot);
if (grub_errno)
- return grub_errno;
+ return NULL;
if (!dev)
continue;
@@ -1019,16 +1019,16 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
if (err)
{
cryptodisk_close (dev);
- return err;
+ return NULL;
}
grub_cryptodisk_insert (dev, name, source);
- have_it = 1;
-
- return GRUB_ERR_NONE;
+ return dev;
}
- return GRUB_ERR_NONE;
+
+ grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
+ return NULL;
}
#ifdef GRUB_UTIL
@@ -1082,8 +1082,10 @@ static int
grub_cryptodisk_scan_device (const char *name,
void *data __attribute__ ((unused)))
{
- grub_err_t err;
+ int ret = 0;
grub_disk_t source;
+ grub_cryptodisk_t dev;
+ grub_errno = GRUB_ERR_NONE;
/* Try to open disk. */
source = grub_disk_open (name);
@@ -1093,13 +1095,26 @@ grub_cryptodisk_scan_device (const char *name,
return 0;
}
- err = grub_cryptodisk_scan_device_real (name, source);
+ dev = grub_cryptodisk_scan_device_real (name, source);
+ if (dev)
+ {
+ ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
+ goto cleanup;
+ }
- grub_disk_close (source);
-
- if (err)
+ /*
+ * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many unhelpful
+ * error messages.
+ */
+ if (grub_errno == GRUB_ERR_BAD_MODULE)
+ grub_error_pop ();
+
+ if (grub_errno != GRUB_ERR_NONE)
grub_print_error ();
- return have_it && search_uuid ? 1 : 0;
+
+ cleanup:
+ grub_disk_close (source);
+ return ret;
}
static grub_err_t
@@ -1110,9 +1125,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 +1140,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;
}
@@ -1142,7 +1157,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
}
else
{
- grub_err_t err;
grub_disk_t disk;
grub_cryptodisk_t dev;
char *diskname;
@@ -1178,13 +1192,13 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- err = grub_cryptodisk_scan_device_real (diskname, disk);
+ dev = grub_cryptodisk_scan_device_real (diskname, disk);
grub_disk_close (disk);
if (disklast)
*disklast = ')';
- return err;
+ return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
}
}
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global
2021-12-09 17:14 ` [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global Glenn Washburn
@ 2021-12-16 19:11 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-16 19:11 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:51AM -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 or
> a cryptodisk of the requested uuid is already open.
>
> To implement this grub_cryptodisk_scan_device_real is modified to return a
> cryptodisk or NULL on failure and having the appropriate grub_errno set to
> indicated failure. Note that grub_cryptodisk_scan_device_real will fail now
> with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
> modules succeed in identifying the source disk.
>
> 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
2021-12-09 17:14 ` [PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules Glenn Washburn
2021-12-09 17:14 ` [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-16 19:12 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations Glenn Washburn
` (6 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
This displays an error notifying the user that they'll want to load a
backend module to make cryptomount useful.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9df3d310f..27491871a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1125,6 +1125,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");
+ if (grub_cryptodisk_list == NULL)
+ return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
+
if (state[0].set)
{
int found_uuid;
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (2 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-16 19:20 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message Glenn Washburn
` (5 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Update such that "cryptomount -u UUID" will not print two error messages
when an invalid passphrase is given and the most relevant error message
will be displayed.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 27491871a..f71bc62eb 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1109,7 +1109,10 @@ grub_cryptodisk_scan_device (const char *name,
if (grub_errno == GRUB_ERR_BAD_MODULE)
grub_error_pop ();
- if (grub_errno != GRUB_ERR_NONE)
+ if (search_uuid != NULL)
+ /* Push error onto stack to save for cryptomount */
+ grub_error_push ();
+ else
grub_print_error ();
cleanup:
@@ -1146,9 +1149,19 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
search_uuid = NULL;
- if (!found_uuid)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
- return GRUB_ERR_NONE;
+ if (found_uuid)
+ return GRUB_ERR_NONE;
+ else if (grub_errno == GRUB_ERR_NONE)
+ {
+ /*
+ * Try to pop the next error on the stack. If there is not one, then
+ * no device matched the given UUID.
+ */
+ grub_error_pop ();
+ if (grub_errno == GRUB_ERR_NONE)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
+ }
+ return grub_errno;
}
else if (state[1].set || (argc == 0 && state[2].set))
{
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (3 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-16 19:21 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
` (4 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
When a cryptmount is specified with a UUID, but no cryptodisk backends find
a disk with that UUID, return a more detailed message giving telling the
user that they might not have a needed cryptobackend module loaded.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f71bc62eb..a5f1b0978 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1159,7 +1159,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
*/
grub_error_pop ();
if (grub_errno == GRUB_ERR_NONE)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found, perhaps a needed disk or cryptodisk module is not loaded");
}
return grub_errno;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message
2021-12-09 17:14 ` [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message Glenn Washburn
@ 2021-12-16 19:21 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-16 19:21 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:54AM -0600, Glenn Washburn wrote:
> When a cryptmount is specified with a UUID, but no cryptodisk backends find
> a disk with that UUID, return a more detailed message giving telling the
> user that they might not have a needed cryptobackend module loaded.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (4 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-20 22:18 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk Glenn Washburn
` (3 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Previously, the cryptomount arguments were passed by global variable and
function call argument, neither of which are ideal. This change passes data
via a grub_cryptomount_args struct, which can be added to over time as
opposed to continually adding arguments to the cryptodisk scan and
recover_key.
As an example, passing a password as a cryptomount argument is implemented.
However, the backends are not implemented, so testing this will return a not
implemented error.
Also, add comments to cryptomount argument parsing to make it more obvious
which argument states are being handled.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
grub-core/disk/geli.c | 6 +++++-
grub-core/disk/luks.c | 7 ++++++-
grub-core/disk/luks2.c | 7 ++++++-
include/grub/cryptodisk.h | 9 ++++++++-
5 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index a5f1b0978..f1ccfbee3 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
/* TRANSLATORS: It's still restricted to cryptodisks only. */
{"all", 'a', 0, N_("Mount all."), 0, 0},
{"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
+ {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
{0, 0, 0, 0, 0, 0}
};
@@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
}
static grub_cryptodisk_t
-grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
+grub_cryptodisk_scan_device_real (const char *name,
+ grub_disk_t source,
+ grub_cryptomount_args_t cargs)
{
grub_err_t err;
grub_cryptodisk_t dev;
@@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
if (!dev)
continue;
- err = cr->recover_key (source, dev);
+ err = cr->recover_key (source, dev, cargs);
if (err)
{
cryptodisk_close (dev);
@@ -1080,11 +1083,12 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
static int
grub_cryptodisk_scan_device (const char *name,
- void *data __attribute__ ((unused)))
+ void *data)
{
int ret = 0;
grub_disk_t source;
grub_cryptodisk_t dev;
+ grub_cryptomount_args_t cargs = data;
grub_errno = GRUB_ERR_NONE;
/* Try to open disk. */
@@ -1095,7 +1099,7 @@ grub_cryptodisk_scan_device (const char *name,
return 0;
}
- dev = grub_cryptodisk_scan_device_real (name, source);
+ dev = grub_cryptodisk_scan_device_real (name, source, cargs);
if (dev)
{
ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
@@ -1124,6 +1128,7 @@ static grub_err_t
grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
struct grub_arg_list *state = ctxt->state;
+ struct grub_cryptomount_args cargs = {0};
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
@@ -1131,7 +1136,13 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
if (grub_cryptodisk_list == NULL)
return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
- if (state[0].set)
+ if (state[3].set) /* password */
+ {
+ cargs.key_data = (grub_uint8_t *) state[3].arg;
+ cargs.key_len = grub_strlen (state[3].arg);
+ }
+
+ if (state[0].set) /* uuid */
{
int found_uuid;
grub_cryptodisk_t dev;
@@ -1146,7 +1157,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
check_boot = state[2].set;
search_uuid = args[0];
- found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+ found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
search_uuid = NULL;
if (found_uuid)
@@ -1163,11 +1174,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
}
return grub_errno;
}
- else if (state[1].set || (argc == 0 && state[2].set))
+ else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
search_uuid = NULL;
check_boot = state[2].set;
- grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+ grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
search_uuid = NULL;
return GRUB_ERR_NONE;
}
@@ -1208,7 +1219,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- dev = grub_cryptodisk_scan_device_real (diskname, disk);
+ dev = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
grub_disk_close (disk);
if (disklast)
@@ -1347,7 +1358,7 @@ GRUB_MOD_INIT (cryptodisk)
{
grub_disk_dev_register (&grub_cryptodisk_dev);
cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
- N_("SOURCE|-u UUID|-a|-b"),
+ N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
N_("Mount a crypto device."), options);
grub_procfs_register ("luks_script", &luks_script);
}
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 2f34a35e6..777da3a05 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
}
static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev)
+recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
{
grub_size_t keysize;
grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -414,6 +414,10 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
grub_disk_addr_t sector;
grub_err_t err;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
return grub_error (GRUB_ERR_BUG, "cipher block is too long");
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 13103ea6a..c5858fcf8 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
static grub_err_t
luks_recover_key (grub_disk_t source,
- grub_cryptodisk_t dev)
+ grub_cryptodisk_t dev,
+ grub_cryptomount_args_t cargs)
{
struct grub_luks_phdr header;
grub_size_t keysize;
@@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
grub_size_t max_stripes = 1;
char *tmp;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
return err;
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index fea196dd4..2cbec8acc 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -545,7 +545,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
static grub_err_t
luks2_recover_key (grub_disk_t source,
- grub_cryptodisk_t crypt)
+ grub_cryptodisk_t crypt,
+ grub_cryptomount_args_t cargs)
{
grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
char passphrase[MAX_PASSPHRASE], cipher[32];
@@ -559,6 +560,10 @@ luks2_recover_key (grub_disk_t source,
grub_json_t *json = NULL, keyslots;
grub_err_t ret;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
ret = luks2_read_header (source, &header);
if (ret)
return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index dcf17fbb3..282f8ac45 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -66,6 +66,13 @@ typedef gcry_err_code_t
(*grub_cryptodisk_rekey_func_t) (struct grub_cryptodisk *dev,
grub_uint64_t zoneno);
+struct grub_cryptomount_args
+{
+ grub_uint8_t *key_data;
+ grub_size_t key_len;
+};
+typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
+
struct grub_cryptodisk
{
struct grub_cryptodisk *next;
@@ -119,7 +126,7 @@ struct grub_cryptodisk_dev
grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
int boot_only);
- grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
+ grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
2021-12-09 17:14 ` [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
@ 2021-12-20 22:18 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-20 22:18 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:55AM -0600, Glenn Washburn wrote:
> Previously, the cryptomount arguments were passed by global variable and
> function call argument, neither of which are ideal. This change passes data
> via a grub_cryptomount_args struct, which can be added to over time as
> opposed to continually adding arguments to the cryptodisk scan and
> recover_key.
>
> As an example, passing a password as a cryptomount argument is implemented.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
>
> Also, add comments to cryptomount argument parsing to make it more obvious
> which argument states are being handled.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (5 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-20 22:28 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
` (2 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
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.
Add documentation of passphrase option for cryptomount as it is now usable.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
docs/grub.texi | 9 ++++--
grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++--------
grub-core/disk/geli.c | 26 ++++-------------
grub-core/disk/luks.c | 27 +++---------------
grub-core/disk/luks2.c | 26 ++++-------------
include/grub/cryptodisk.h | 1 +
6 files changed, 66 insertions(+), 79 deletions(-)
diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..e0a309574 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4307,13 +4307,16 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See command @command{hashsum}
@node cryptomount
@subsection cryptomount
-@deffn Command cryptomount device|@option{-u} uuid|@option{-a}|@option{-b}
-Setup access to encrypted device. If necessary, passphrase
-is requested interactively. Option @var{device} configures specific grub device
+@deffn Command cryptomount [@option{-p} password] device|@option{-u} uuid|@option{-a}|@option{-b}
+Setup access to encrypted device. If @option{-p} is not given, a passphrase
+is requested interactively. Otherwise, the given @var{password} will be used and
+no passphrase will be requested interactively.
+Option @var{device} configures specific grub device
(@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
with specified @var{uuid}; option @option{-a} configures all detected encrypted
devices; option @option{-b} configures all geli containers that have boot flag set.
+
GRUB suports devices encrypted using LUKS, LUKS2 and geli. Note that necessary
modules (@var{luks}, @var{luks2} and @var{geli}) have to be loaded manually
before this command can be used. For LUKS2 only the PBKDF2 key derivation
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f1ccfbee3..f8f6e3e25 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,53 @@ grub_cryptodisk_scan_device_real (const char *name,
return NULL;
if (!dev)
continue;
-
- err = cr->recover_key (source, dev, cargs);
- if (err)
- {
- cryptodisk_close (dev);
- return NULL;
- }
+
+ if (!cargs->key_len)
+ {
+ /* Get the passphrase from the user, if no key data. */
+ askpass = 1;
+ if (source->partition != NULL)
+ 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 : "",
+ dev->uuid);
+ grub_free (part);
+
+ cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+ if (cargs->key_data == NULL)
+ return NULL;
+
+ 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)
+ goto error;
grub_cryptodisk_insert (dev, name, source);
- return dev;
+ goto cleanup;
}
-
grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
- return NULL;
+ goto cleanup;
+
+ error:
+ cryptodisk_close (dev);
+ dev = NULL;
+
+ cleanup:
+ if (askpass)
+ {
+ cargs->key_len = 0;
+ grub_free (cargs->key_data);
+ }
+ return dev;
}
#ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 777da3a05..7299a47d1 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -135,8 +135,6 @@ const char *algorithms[] = {
[0x16] = "aes"
};
-#define MAX_PASSPHRASE 256
-
static gcry_err_code_t
geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno)
{
@@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN];
grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE];
grub_uint8_t geli_cipher_key[64];
- char passphrase[MAX_PASSPHRASE] = "";
unsigned i;
gcry_err_code_t gcry_err;
struct grub_geli_phdr header;
- char *tmp;
grub_disk_addr_t sector;
grub_err_t err;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || cargs->key_len == 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
return grub_error (GRUB_ERR_BUG, "cipher block is too long");
@@ -438,23 +433,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
grub_puts_ (N_("Attempting to decrypt master key..."));
- /* Get the passphrase from the user. */
- tmp = NULL;
- if (source->partition)
- tmp = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", tmp ? : "",
- dev->uuid);
- grub_free (tmp);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-
/* Calculate the PBKDF2 of the user supplied passphrase. */
if (grub_le_to_cpu32 (header.niter) != 0)
{
grub_uint8_t pbkdf_key[64];
- gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
- grub_strlen (passphrase),
+ gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
+ cargs->key_len,
header.salt,
sizeof (header.salt),
grub_le_to_cpu32 (header.niter),
@@ -477,7 +461,7 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
return grub_crypto_gcry_error (GPG_ERR_OUT_OF_MEMORY);
grub_crypto_hmac_write (hnd, header.salt, sizeof (header.salt));
- grub_crypto_hmac_write (hnd, passphrase, grub_strlen (passphrase));
+ grub_crypto_hmac_write (hnd, cargs->key_data, cargs->key_len);
gcry_err = grub_crypto_hmac_fini (hnd, geomkey);
if (gcry_err)
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index c5858fcf8..39a7af6a4 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -29,8 +29,6 @@
GRUB_MOD_LICENSE ("GPLv3+");
-#define MAX_PASSPHRASE 256
-
#define LUKS_KEY_ENABLED 0x00AC71F3
/* On disk LUKS header */
@@ -158,17 +156,14 @@ luks_recover_key (grub_disk_t source,
struct grub_luks_phdr header;
grub_size_t keysize;
grub_uint8_t *split_key = NULL;
- char passphrase[MAX_PASSPHRASE] = "";
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
unsigned i;
grub_size_t length;
grub_err_t err;
grub_size_t max_stripes = 1;
- char *tmp;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || cargs->key_len == 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
@@ -188,20 +183,6 @@ luks_recover_key (grub_disk_t source,
if (!split_key)
return grub_errno;
- /* Get the passphrase from the user. */
- tmp = NULL;
- if (source->partition)
- tmp = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", tmp ? : "",
- dev->uuid);
- grub_free (tmp);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- {
- grub_free (split_key);
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
- }
-
/* Try to recover master key from each active keyslot. */
for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
{
@@ -216,8 +197,8 @@ luks_recover_key (grub_disk_t source,
grub_dprintf ("luks", "Trying keyslot %d\n", i);
/* Calculate the PBKDF2 of the user supplied passphrase. */
- gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
- grub_strlen (passphrase),
+ gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
+ cargs->key_len,
header.keyblock[i].passwordSalt,
sizeof (header.keyblock[i].passwordSalt),
grub_be_to_cpu32 (header.keyblock[i].
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 2cbec8acc..e6f9a22f8 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -35,8 +35,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
#define LUKS_MAGIC_1ST "LUKS\xBA\xBE"
#define LUKS_MAGIC_2ND "SKUL\xBA\xBE"
-#define MAX_PASSPHRASE 256
-
enum grub_luks2_kdf_type
{
LUKS2_KDF_TYPE_ARGON2I,
@@ -549,8 +547,8 @@ luks2_recover_key (grub_disk_t source,
grub_cryptomount_args_t cargs)
{
grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
- char passphrase[MAX_PASSPHRASE], cipher[32];
- char *json_header = NULL, *part = NULL, *ptr;
+ char cipher[32];
+ char *json_header = NULL, *ptr;
grub_size_t candidate_key_len = 0, json_idx, size;
grub_luks2_header_t header;
grub_luks2_keyslot_t keyslot;
@@ -560,9 +558,8 @@ luks2_recover_key (grub_disk_t source,
grub_json_t *json = NULL, keyslots;
grub_err_t ret;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || cargs->key_len == 0)
+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
ret = luks2_read_header (source, &header);
if (ret)
@@ -589,18 +586,6 @@ luks2_recover_key (grub_disk_t source,
goto err;
}
- /* Get the passphrase from the user. */
- 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 ? : "",
- crypt->uuid);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- {
- ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
- goto err;
- }
-
if (grub_json_getvalue (&keyslots, json, "keyslots") ||
grub_json_getsize (&size, &keyslots))
{
@@ -725,7 +710,7 @@ luks2_recover_key (grub_disk_t source,
}
ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
- (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
+ cargs->key_data, cargs->key_len);
if (ret)
{
grub_dprintf ("luks2", "Decryption with keyslot \"%" PRIuGRUB_UINT64_T "\" failed: %s\n",
@@ -777,7 +762,6 @@ luks2_recover_key (grub_disk_t source,
}
err:
- grub_free (part);
grub_free (json_header);
grub_json_free (json);
return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 282f8ac45..5bd970692 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -59,6 +59,7 @@ typedef enum
#define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE - 3)
#define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
#define GRUB_CRYPTODISK_MAX_KEYLEN 128
+#define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
struct grub_cryptodisk;
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
2021-12-09 17:14 ` [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk Glenn Washburn
@ 2021-12-20 22:28 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-20 22:28 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:56AM -0600, 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.
>
> Add documentation of passphrase option for cryptomount as it is now usable.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (6 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-20 22:36 ` Daniel Kiper
2021-12-09 17:14 ` [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt Glenn Washburn
2021-12-20 22:40 ` [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Daniel Kiper
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Note that cargs.search_uuid does not need to be initialized in various parts
of the cryptomount argument parsing, just once when cargs is declared with a
struct initializer. The previous code used a global variable which would
retain the value across cryptomount invocations.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 24 +++++++++---------------
grub-core/disk/geli.c | 9 ++++-----
grub-core/disk/luks.c | 9 ++++-----
grub-core/disk/luks2.c | 8 ++++----
include/grub/cryptodisk.h | 9 +++++++--
5 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f8f6e3e25..99265097a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
#endif
-static int check_boot;
-static char *search_uuid;
-
static void
cryptodisk_close (grub_cryptodisk_t dev)
{
@@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
FOR_CRYPTODISK_DEVS (cr)
{
- dev = cr->scan (source, search_uuid, check_boot);
+ dev = cr->scan (source, cargs);
if (grub_errno)
return NULL;
if (!dev)
@@ -1077,6 +1074,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
grub_cryptodisk_t dev;
grub_cryptodisk_dev_t cr;
grub_disk_t source;
+ struct grub_cryptomount_args cargs = {0};
/* Try to open disk. */
source = grub_disk_open (sourcedev);
@@ -1093,7 +1091,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
FOR_CRYPTODISK_DEVS (cr)
{
- dev = cr->scan (source, search_uuid, check_boot);
+ dev = cr->scan (source, &cargs);
if (grub_errno)
return grub_errno;
if (!dev)
@@ -1136,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
dev = grub_cryptodisk_scan_device_real (name, source, cargs);
if (dev)
{
- ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
+ ret = (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, dev->uuid) == 0);
goto cleanup;
}
@@ -1147,7 +1145,7 @@ grub_cryptodisk_scan_device (const char *name,
if (grub_errno == GRUB_ERR_BAD_MODULE)
grub_error_pop ();
- if (search_uuid != NULL)
+ if (cargs->search_uuid != NULL)
/* Push error onto stack to save for cryptomount */
grub_error_push ();
else
@@ -1189,10 +1187,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- check_boot = state[2].set;
- search_uuid = args[0];
+ cargs.check_boot = state[2].set;
+ cargs.search_uuid = args[0];
found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
- search_uuid = NULL;
if (found_uuid)
return GRUB_ERR_NONE;
@@ -1210,10 +1207,8 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
}
else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
- search_uuid = NULL;
return GRUB_ERR_NONE;
}
else
@@ -1224,8 +1219,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
char *disklast = NULL;
grub_size_t len;
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
diskname = args[0];
len = grub_strlen (diskname);
if (len && diskname[0] == '(' && diskname[len - 1] == ')')
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 7299a47d1..23789c43f 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
#endif
static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int boot_only)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t newdev;
struct grub_geli_phdr header;
@@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
+ if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
{
grub_dprintf ("geli", "not a boot volume\n");
return NULL;
@@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
{
- grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
+ grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
return NULL;
}
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 39a7af6a4..f0feb3844 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,8 +63,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
grub_size_t blocknumbers);
static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int check_boot)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t newdev;
const char *iptr;
@@ -76,7 +75,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
char hashspec[sizeof (header.hashSpec) + 1];
grub_err_t err;
- if (check_boot)
+ if (cargs->check_boot)
return NULL;
/* Read the LUKS header. */
@@ -103,9 +102,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
}
*optr = 0;
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
{
- grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+ grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
return NULL;
}
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e6f9a22f8..6d7d1f0d0 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -346,14 +346,14 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
}
static grub_cryptodisk_t
-luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
+luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t cryptodisk;
grub_luks2_header_t header;
char uuid[sizeof (header.uuid) + 1];
grub_size_t i, j;
- if (check_boot)
+ if (cargs->check_boot)
return NULL;
if (luks2_read_header (disk, &header))
@@ -367,9 +367,9 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
uuid[j++] = header.uuid[i];
uuid[j] = '\0';
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
{
- grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
+ grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
return NULL;
}
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 5bd970692..c6524c9ea 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -69,7 +69,13 @@ typedef gcry_err_code_t
struct grub_cryptomount_args
{
+ /* scan: Flag to indicate that only bootable volumes should be decrypted */
+ grub_uint32_t check_boot : 1;
+ /* scan: Only volumes matching this UUID should be decrpyted */
+ char *search_uuid;
+ /* recover_key: Key data used to decrypt voume */
grub_uint8_t *key_data;
+ /* recover_key: Length of key_data */
grub_size_t key_len;
};
typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
@@ -125,8 +131,7 @@ struct grub_cryptodisk_dev
struct grub_cryptodisk_dev *next;
struct grub_cryptodisk_dev **prev;
- grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
- int boot_only);
+ grub_cryptodisk_t (*scan) (grub_disk_t disk, grub_cryptomount_args_t cargs);
grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-12-09 17:14 ` [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
@ 2021-12-20 22:36 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-20 22:36 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:57AM -0600, Glenn Washburn wrote:
> Note that cargs.search_uuid does not need to be initialized in various parts
> of the cryptomount argument parsing, just once when cargs is declared with a
> struct initializer. The previous code used a global variable which would
> retain the value across cryptomount invocations.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (7 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
@ 2021-12-09 17:14 ` Glenn Washburn
2021-12-20 22:37 ` Daniel Kiper
2021-12-20 22:40 ` [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Daniel Kiper
9 siblings, 1 reply; 21+ messages in thread
From: Glenn Washburn @ 2021-12-09 17:14 UTC (permalink / raw)
To: Daniel Kiper, grub-devel
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Call grub_partition_get_name unconditionally to initialize the part
variable. Then part will only be NULL when grub_partition_get_name errors.
Note that when source->partition is NULL, then grub_partition_get_name
returns an allocated empty string. So no comma or partition will be printed,
as desired.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 99265097a..2381c3330 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1021,11 +1021,10 @@ grub_cryptodisk_scan_device_real (const char *name,
{
/* Get the passphrase from the user, if no key data. */
askpass = 1;
- if (source->partition != NULL)
- part = grub_partition_get_name (source->partition);
+ 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 : "",
+ part != NULL ? part : N_("UNKNOWN"),
dev->uuid);
grub_free (part);
--
2.27.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt
2021-12-09 17:14 ` [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt Glenn Washburn
@ 2021-12-20 22:37 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-20 22:37 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:58AM -0600, Glenn Washburn wrote:
> Call grub_partition_get_name unconditionally to initialize the part
> variable. Then part will only be NULL when grub_partition_get_name errors.
> Note that when source->partition is NULL, then grub_partition_get_name
> returns an allocated empty string. So no comma or partition will be printed,
> as desired.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules
2021-12-09 17:14 [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (8 preceding siblings ...)
2021-12-09 17:14 ` [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt Glenn Washburn
@ 2021-12-20 22:40 ` Daniel Kiper
2021-12-23 23:39 ` Daniel Kiper
9 siblings, 1 reply; 21+ messages in thread
From: Daniel Kiper @ 2021-12-20 22:40 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Thu, Dec 09, 2021 at 11:14:49AM -0600, Glenn Washburn wrote:
> Updates since v4:
> * Rework patch #2 to (hopefully) be easier to understand
> * Add more commentary to patch #2 commit message
> * Split previous patch #3 into three separate patches
>
> ---
> This patch series refactors the way cryptomount passes data to the crypto
> modules. Currently, the method has been by global variable and function call
> argument, neither of which are ideal. This method passes data via a
> grub_cryptomount_args struct, which can be added to over time as opposed to
> continually adding arguments to the cryptodisk recover_key (as is being
> proposed in the keyfile and detached header patches).
Whole patch series looks good to me. I am going to merge it no later
than on Thursday this week.
Patrick, could you do final check of these patches before I will apply them?
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules
2021-12-20 22:40 ` [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Daniel Kiper
@ 2021-12-23 23:39 ` Daniel Kiper
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Kiper @ 2021-12-23 23:39 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley
On Mon, Dec 20, 2021 at 11:40:10PM +0100, Daniel Kiper wrote:
> On Thu, Dec 09, 2021 at 11:14:49AM -0600, Glenn Washburn wrote:
> > Updates since v4:
> > * Rework patch #2 to (hopefully) be easier to understand
> > * Add more commentary to patch #2 commit message
> > * Split previous patch #3 into three separate patches
> >
> > ---
> > This patch series refactors the way cryptomount passes data to the crypto
> > modules. Currently, the method has been by global variable and function call
> > argument, neither of which are ideal. This method passes data via a
> > grub_cryptomount_args struct, which can be added to over time as opposed to
> > continually adding arguments to the cryptodisk recover_key (as is being
> > proposed in the keyfile and detached header patches).
>
> Whole patch series looks good to me. I am going to merge it no later
> than on Thursday this week.
I have applied the patch set conditionally due to this Coverity report:
*** CID 366905: Memory - illegal accesses (USE_AFTER_FREE)
/grub-core/disk/cryptodisk.c: 1064 in grub_cryptodisk_scan_device_real()
1058 cleanup:
1059 if (askpass)
1060 {
1061 cargs->key_len = 0;
1062 grub_free (cargs->key_data);
1063 }
>>> CID 366905: Memory - illegal accesses (USE_AFTER_FREE)
>>> Using freed pointer "dev".
1064 return dev;
1065 }
1066
1067 #ifdef GRUB_UTIL
1068 #include <grub/util/misc.h>
1069 grub_err_t
Glenn, please fix this after holiday.
James, I think you can rabase your patch set on the latest master now.
The issue mentioned above should not impact your work.
Daniel
^ permalink raw reply [flat|nested] 21+ messages in thread