Re: [PATCH 0/2] Fix some Coverity resource leak bugs
On Wed, Nov 10, 2021 at 03:49:27PM -0500, Alec Brown wrote: > There were a couple of commits which made fixes to a few Coverity bugs > addressing resource leaks, but missed some cases where a variable was not > being > freed before a return statement was called. > > The Coverity Bugs being addressed are: > CID 292443 > CID 73804 > > Alec Brown (2): > commands/probe: Fix resource leaks > disk/ldm: Fix resource leak Reviewed-by: Daniel Kiper for both... Thank you for fixing these issues. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.
I am not very happy with that. Splitting this into separate patch or
merging with patch #2 probably would not help either.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
The commit message lacks of explanation why this change is needed.
I think you can copy part of the cover letter here.
> Signed-off-by: Glenn Washburn
> ---
> 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 90f82b2d3..577942088 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},
Should not you update docs/grub.texi as well?
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> }
>
> static grub_err_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,10 +1083,11 @@ 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)
> {
>grub_err_t err;
>grub_disk_t source;
> + grub_cryptomount_args_t cargs = data;
>
>/* Try to open disk. */
>source = grub_disk_open (name);
> @@ -1093,7 +1097,7 @@ grub_cryptodisk_scan_device (const char *name,
>return 0;
> }
>
> - err = grub_cryptodisk_scan_device_real (name, source);
> + err = grub_cryptodisk_scan_device_real (name, source, cargs);
>
>grub_disk_close (source);
>
> @@ -1106,12 +1110,19 @@ 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");
>
> + if (state[3].set) /* password */
> +{
> + cargs.key_data = (grub_uint8_t *) state[3].arg;
> + cargs.key_len = grub_strlen (state[3].arg);
> +}
> +
>have_it = 0;
> - if (state[0].set)
> + if (state[0].set) /* uuid */
Nice but if you want to do that please put that change into separate patch.
Or at least advise in the commit message you are doing this on occasion.
> {
>grub_cryptodisk_t dev;
>
> @@ -1125,18 +1136,18 @@ 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);
> + grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
>search_uuid = NULL;
>
>if (!have_it)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>return GRUB_ERR_NONE;
> }
> - else if (state[1].set || (argc == 0 && state[2].set))
> + else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
Ditto.
> {
>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;
> }
> @@ -1178,7 +1189,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> return GRUB_ERR_NONE;
> }
>
> - err = grub_cryptodisk_scan_device_real (diskname, disk);
> + err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
>
>grub_disk_close (disk);
>if (disklast)
> @@ -1317,7 +1328,7 @@ GRUB_MOD_INIT (cryptodisk)
> {
>grub_disk_dev_register (&grub_cryptodisk_dev);
>cmd = grub_register_extcmd ("c
Re: [PATCH v3 2/4] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> The crypto device modules should only be setting up the crypto devices and
> not getting user input. This has the added benefit of simplifying the code
> such that three essentially duplicate pieces of code are merged into one.
Mostly nitpicks below...
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 52 ++---
> grub-core/disk/geli.c | 26 ---
> grub-core/disk/luks.c | 27 +++
> grub-core/disk/luks2.c | 26 ---
> include/grub/cryptodisk.h | 1 +
> 5 files changed, 57 insertions(+), 75 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 577942088..a5f7b860c 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
> grub_disk_t source,
> grub_cryptomount_args_t cargs)
> {
> - grub_err_t err;
> + grub_err_t ret = GRUB_ERR_NONE;
>grub_cryptodisk_t dev;
>grub_cryptodisk_dev_t cr;
> + int askpass = 0;
> + char *part = NULL;
>
>dev = grub_cryptodisk_get_by_source_disk (source);
>
> @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
>return grub_errno;
> if (!dev)
>continue;
> -
> -err = cr->recover_key (source, dev, cargs);
> -if (err)
> -{
> - cryptodisk_close (dev);
> - return err;
> -}
> +
> +if (cargs->key_len == 0)
I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> + {
> + /* Get the passphrase from the user, if no key data. */
> + askpass = 1;
> + if (source->partition)
...but not for the pointers and enums.
s/if (source->partition)/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 ? "," : "", part ? : "",
s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
s/part ? : ""/part != NULL ? part : "NO NAME"/?
> + dev->uuid);
> + grub_free (part);
> +
> + cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + if (!cargs->key_data)
Ditto and below please...
> + return grub_errno;
> +
> + if (!grub_password_get ((char *) cargs->key_data,
> GRUB_CRYPTODISK_MAX_PASSPHRASE))
> + {
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
All errors printed by grub_error() should start with lower case...
> + goto error;
> + }
> + cargs->key_len = grub_strlen ((char *) cargs->key_data);
> + }
> +
> +ret = cr->recover_key (source, dev, cargs);
> +if (ret)
if (ret != GRUB_ERR_NONE)
> + goto error;
>
> grub_cryptodisk_insert (dev, name, source);
>
> have_it = 1;
>
> -return GRUB_ERR_NONE;
> +goto cleanup;
>}
> - return GRUB_ERR_NONE;
> + goto cleanup;
> +
> +error:
Please put space before labels.
> + cryptodisk_close (dev);
I would add empty line here.
> +cleanup:
Please put space before labels.
> + if (askpass)
> +{
> + cargs->key_len = 0;
> + grub_free (cargs->key_data);
> +}
> + return ret;
> }
>
> #ifdef GRUB_UTIL
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index 4e8c377e7..32f34d5c3 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 || 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");
All errors printed by grub_error() should start with lower case...
>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)
> -t
