From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1p0cax-0007fZ-4Z for mharc-grub-devel@gnu.org; Thu, 01 Dec 2022 00:58:52 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0cat-0007fO-Km for grub-devel@gnu.org; Thu, 01 Dec 2022 00:58:48 -0500 Received: from mail-vk1-xa2f.google.com ([2607:f8b0:4864:20::a2f]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1p0caq-0004xN-CY for grub-devel@gnu.org; Thu, 01 Dec 2022 00:58:47 -0500 Received: by mail-vk1-xa2f.google.com with SMTP id o24so355691vkl.9 for ; Wed, 30 Nov 2022 21:58:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to; bh=KBB/5C3hNkxMYdJPYzs6Rdc2XjP+uJxY8M0bqq6uJLk=; b=uOKORPI2t1J6tSMmBY4cm28M426A5s3bg1JkserfnCfKXnsmbUq+MqgPDw8BYwlaIT PjQSSPEsUL/XWlLUTvOJXZL9Rq+3c4vo0x0LzcRwOJVSnXoVRRT++Xc9JxlFawdwpQwp ICFFz7qVAxMRP94jdFI2btp5MahgQqS/E5HD0lmhzUxvpRPrMUWoDVorftIrLs8iNoIN kuqiTZTDh6Z8kKBEOOuUmrW65FXc1Wr8UjWzeNlQ3q43VvUhdz1P7JY44HzmEPfS7kLA xl2T4MQKTnqFaY0D5zKpeTXzDvnJ7XVnB2+4OlKiuluv3JqK2VHLMyy4y8RM4v+knmlv A8pA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:reply-to:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=KBB/5C3hNkxMYdJPYzs6Rdc2XjP+uJxY8M0bqq6uJLk=; b=ML0PaQ6pMhJ6u7XbG3vaw7f60TvKGp0Ubo1dS4W9wFDTQe8S6edMvHQu4pcjxx4bD5 qC8x2KzAt1GZlTM+EO5q2YEZXjJVtdtzne0FGILbILyT8zbJFjDAd604KCJsP+b9Gsow 6drnubv8wHPTRlG2d0BIvJIaCUkrtLWsil1DXvwjEihpsR6QtoFc1vjTeg1W0u38U95x Fzo8afhLqDPWYMbm4pEkyd3UPJ+rZ2pSRYOF50lQiaj7Zd+NfHGXuLp2mICpHMF64CQX jVQyKCbm5f4/Fivv8osdKVw+BaHzh00qfZyILZnwglmS/qFznU1Ugi+06upWU5GBVCBT uJSQ== X-Gm-Message-State: ANoB5pnUK6nwt+bv+3lq2LH74+aYgY7n80DZ+urhWfSN74hXhoU+DFHj megtjBIYo1AeHLMkYxReQjdlDbyIouC67w== X-Google-Smtp-Source: AA0mqf75Dsp291lo06aKq+LLU/8DIK2rMCp4Gj8W9SR+s8Xq44c46jzpKaqkORmRVV62rnsFNFDe0A== X-Received: by 2002:a05:6122:f88:b0:3bc:da16:c70 with SMTP id br8-20020a0561220f8800b003bcda160c70mr11184501vkb.12.1669874320850; Wed, 30 Nov 2022 21:58:40 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id 1-20020a1f1901000000b003bbfc4cb34esm493671vkz.25.2022.11.30.21.58.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Nov 2022 21:58:40 -0800 (PST) Date: Wed, 30 Nov 2022 23:58:33 -0600 From: Glenn Washburn To: Daniel Kiper Cc: Maxim Fomin , "grub-devel@gnu.org" , "ps@pks.im" Subject: Re: [PATCH v8 1/1] plainmount: Support plain encryption mode Message-ID: <20221130235833.7bf20dc5@crass-HP-ZBook-15-G2> In-Reply-To: <20221129171311.k5yrlmmt4c4bo4r5@tomti.i.net-space.pl> References: <20221129171311.k5yrlmmt4c4bo4r5@tomti.i.net-space.pl> Reply-To: development@efficientek.com X-Mailer: Claws Mail 4.1.0 (GTK 3.24.34; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::a2f; envelope-from=development@efficientek.com; helo=mail-vk1-xa2f.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Dec 2022 05:58:48 -0000 On Tue, 29 Nov 2022 18:13:11 +0100 Daniel Kiper wrote: > First of all, sorry for late reply. I hope I will be able to review > next version of this patch much faster. > > On Sat, Oct 29, 2022 at 05:40:42PM +0000, Maxim Fomin wrote: > > From 2b1d2deb3f2416cbc3e7d25cbc4141a3078eaf68 Mon Sep 17 00:00:00 > > 2001 From: Maxim Fomin > > Date: Sat, 29 Oct 2022 18:18:58 +0100 > > Subject: [PATCH v8 1/1] plainmount: Support plain encryption mode > > > > This patch adds support for plain encryption mode (plain dm-crypt) > > via new module/command named 'plainmount'. > > > > Signed-off-by: Maxim Fomin > > Please put "---" behind SOB and before "Difference with..." next > time... [...] > > + > > +/* Configure cryptodisk uuid */ > > +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char > > *user_uuid) +{ > > + grub_size_t pos = 0; > > + > > + /* Size of user_uuid is checked in main func */ > > + if (user_uuid != NULL) > > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid)); > > I think grub_strcpy() instead of grub_memcpy() would be more natural > in string contexts. And then you do not need grub_strlen(). > > Is it safe to assume here that dev->uuid has been zeroed earlier? Yes, its zalloc'd in grub_cmd_plainmount below. > > > + else > > + { > > + /* > > + * Set default UUID. Last digits start from 1 and are > > incremented for > > + * each new plainmount device by snprintf(). > > + */ > > + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", > > dev->id+1); > > s/sizeof (dev->uuid)-1/sizeof (dev->uuid) - 1/ > s/dev->id+1/dev->id + 1/ > > > + while (dev->uuid[++pos] == ' '); > > + grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos); > > + } > > + COMPILE_TIME_ASSERT (sizeof (dev->uuid) >= sizeof > > (PLAINMOUNT_DEFAULT_UUID)); +} > > + > > + > > +/* Configure cryptodevice sector size (-S option) */ > > +static grub_err_t > > +plainmount_configure_sectors (grub_cryptodisk_t dev, grub_disk_t > > disk, > > + grub_size_t sector_size) > > +{ > > + dev->total_sectors = grub_disk_native_sectors (disk); > > + if (dev->total_sectors == GRUB_DISK_SIZE_UNKNOWN) > > + return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot determine > > disk %s size"), > > + disk->name); > > + > > + /* Convert size to sectors */ > > + dev->log_sector_size = grub_log2ull (sector_size); > > + dev->total_sectors = grub_convert_sector (dev->total_sectors, > > + GRUB_DISK_SECTOR_BITS, > > + dev->log_sector_size); > > + if (dev->total_sectors == 0) > > + return grub_error (GRUB_ERR_BAD_DEVICE, > > + N_("cannot set specified sector size on > > disk %s"), > > + disk->name); > > + > > + grub_dprintf ("plainmount", "log_sector_size=%d, total_sectors=%" > > + PRIuGRUB_SIZE"\n", dev->log_sector_size, > > dev->total_sectors); > > + return GRUB_ERR_NONE; > > +} > > + > > + > > +/* Hashes a password into a key and stores it with the cipher. */ > > +static grub_err_t > > +plainmount_configure_password (grub_cryptodisk_t dev, const char > > *hash, > > + grub_uint8_t *key_data, grub_size_t > > key_size, > > + grub_size_t password_size) > > +{ > > + grub_uint8_t *derived_hash, *dh; > > + char *p; > > + unsigned int round, i, len, size; > > + grub_size_t alloc_size; > > + grub_err_t err = GRUB_ERR_NONE; > > + > > + /* Support none (plain) hash */ > > + if (grub_strcmp (hash, "plain") == 0) > > + { > > + dev->hash = NULL; > > + return err; > > + } > > + > > + /* Hash argument was checked at main func */ > > + dev->hash = grub_crypto_lookup_md_by_name (hash); > > + len = dev->hash->mdlen; > > + > > + alloc_size = grub_max (password_size, key_size); > > + /* > > + * Allocate buffer for the password and for an added prefix > > character > > + * for each hash round ('alloc_size' may not be a multiple of > > 'len'). > > + */ > > + p = grub_zalloc (alloc_size + (alloc_size / len) + 1); > > + derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2); > > + if (p == NULL || derived_hash == NULL) > > + { > > + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory"); > > + goto exit; > > + } > > + dh = derived_hash; > > + > > + /* > > + * Hash password. Adapted from cryptsetup. > > + * > > https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypt_plain.c > > + */ > > + for (round = 0, size = alloc_size; size; round++, dh += len, > > size -= len) > > + { > > + for (i = 0; i < round; i++) > > + p[i] = 'A'; > > + > > + grub_memcpy (p + i, (char*) key_data, password_size); > > + > > + if (len > size) > > + len = size; > > + > > + grub_crypto_hash (dev->hash, dh, p, password_size + round); > > + } > > + grub_memcpy (key_data, derived_hash, key_size); > > + > > +exit: > > I prefer "fail" instead of "exit" label in that context. And labels > should be prefixed with one space. Same applies to "exit" labels > below. > > > + grub_free (p); > > + grub_free (derived_hash); > > + return err; > > +} > > + > > + > > +/* Read key material from keyfile */ > > +static grub_err_t > > +plainmount_configure_keyfile (char *keyfile, grub_uint8_t > > *key_data, > > + grub_size_t key_size, grub_size_t > > keyfile_offset) +{ > > + grub_file_t g_keyfile = grub_file_open (keyfile, > > GRUB_FILE_TYPE_NONE); > > + if (g_keyfile == NULL) > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("cannot open > > keyfile %s"), > > + keyfile); > > + > > + if (grub_file_seek (g_keyfile, keyfile_offset) == (grub_off_t)-1) > > s/(grub_off_t)-1/(grub_off_t) -1/ > Casts require a space after ")"... > > > + return grub_error (GRUB_ERR_FILE_READ_ERROR, > > + N_("cannot seek keyfile at offset > > %"PRIuGRUB_SIZE), > > + keyfile_offset); > > + > > + if (key_size > (g_keyfile->size - keyfile_offset)) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Specified key > > size (%" > > + PRIuGRUB_SIZE") is too small for keyfile > > size (%" > > + PRIuGRUB_SIZE") and offset > > (%"PRIuGRUB_SIZE")"), > > + key_size, g_keyfile->size, keyfile_offset); > > + > > + if (grub_file_read (g_keyfile, key_data, key_size) != > > (grub_ssize_t) key_size) > > + return grub_error (GRUB_ERR_FILE_READ_ERROR, N_("error reading > > key file")); > > + return GRUB_ERR_NONE; > > +} > > + > > + > > +/* Plainmount command entry point */ > > +static grub_err_t > > +grub_cmd_plainmount (grub_extcmd_context_t ctxt, int argc, char > > **args) +{ > > + struct grub_arg_list *state = ctxt->state; > > + grub_cryptodisk_t dev = NULL; > > + grub_disk_t disk = NULL; > > + const gcry_md_spec_t *gcry_hash; > > + char *diskname, *disklast = NULL, *cipher, *mode, *hash, > > *keyfile, *uuid; > > + grub_size_t len, key_size, sector_size, keyfile_offset = 0, > > password_size = 0; > > + grub_err_t err; > > + const char *p; > > + grub_uint8_t *key_data; > > + > > + if (argc < 1) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("device name > > required")); + > > + /* Check whether required arguments are specified */ > > + if (!state[OPTION_CIPHER].set || !state[OPTION_KEY_SIZE].set) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "cipher and key > > size must be set"); > > + if (!state[OPTION_HASH].set && !state[OPTION_KEYFILE].set) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "hash algorithm > > must be set"); + > > + /* Check hash */ > > + if (!state[OPTION_KEYFILE].set) > > + { > > + gcry_hash = grub_crypto_lookup_md_by_name > > (state[OPTION_HASH].arg); > > + if (!gcry_hash) > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("couldn't > > load hash %s"), > > + state[OPTION_HASH].arg); > > + > > + if (gcry_hash->mdlen > GRUB_CRYPTODISK_MAX_KEYLEN) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("hash length %"PRIuGRUB_SIZE" exceeds > > maximum %d bits"), > > + gcry_hash->mdlen * GRUB_CHAR_BIT, > > + GRUB_CRYPTODISK_MAX_KEYLEN * > > GRUB_CHAR_BIT); > > + } > > + > > + /* Check cipher mode */ > > + if (!grub_strchr (state[OPTION_CIPHER].arg,'-')) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("invalid cipher mode, must be of format > > cipher-mode")); + > > + /* Check password size */ > > + if (state[OPTION_PASSWORD].set && grub_strlen > > (state[OPTION_PASSWORD].arg) > > > + > > GRUB_CRYPTODISK_MAX_PASSPHRASE) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("password exceeds maximium size")); > > + > > + /* Check uuid length */ > > + if (state[OPTION_UUID].set && grub_strlen > > (state[OPTION_UUID].arg) > > > + sizeof (PLAINMOUNT_DEFAULT_UUID)) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("specified UUID exceeds maximum size")); > > What will happen if somebody passes shorter, probably non-valid, UUID? > I think we should check UUID is not shorter than something too. Afaik, we don't really care what the UUID string is, just that its unique. It doesn't have to be a real UUID either. A minimum length check of 1 might be good, otherwise '--uuid ""' will create an empty uuid, but its not a big deal either. We might think about a character validation check so that characters like ')' can't be in the string. None of this will cause real problems as far as I can tell, but some features may not work right. For example, having ')' in the uuid will make it so that you can't access the crypto device via the '(cruuid/)' syntax. I also don't mind not doing the validation and letting the user shoot themselves in the foot if they so choose. Glenn > > > + /* Parse plainmount arguments */ > > + grub_errno = GRUB_ERR_NONE; > > + keyfile_offset = state[OPTION_KEYFILE_OFFSET].set ? > > + grub_strtoull > > (state[OPTION_KEYFILE_OFFSET].arg, &p, 0) : 0; > > + if (state[OPTION_KEYFILE_OFFSET].set && > > + (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0' || > > + grub_errno != GRUB_ERR_NONE)) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized > > keyfile offset")); + > > + sector_size = state[OPTION_SECTOR_SIZE].set ? > > + grub_strtoull (state[OPTION_SECTOR_SIZE].arg, &p, > > 0) : > > + PLAINMOUNT_DEFAULT_SECTOR_SIZE; > > + if (state[OPTION_SECTOR_SIZE].set && > > (state[OPTION_SECTOR_SIZE].arg[0] == '\0' || > > + *p != '\0' || grub_errno > > != GRUB_ERR_NONE)) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized > > sector size")); + > > + /* Check key size */ > > + key_size = grub_strtoull (state[OPTION_KEY_SIZE].arg, &p, 0); > > + if (state[OPTION_KEY_SIZE].arg[0] == '\0' || *p != '\0' || > > + grub_errno != GRUB_ERR_NONE) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("unrecognized key > > size")); > > + if ((key_size % GRUB_CHAR_BIT) != 0) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("key size is not multiple of %d bits"), > > GRUB_CHAR_BIT); > > + key_size = key_size / GRUB_CHAR_BIT; > > + if (key_size > GRUB_CRYPTODISK_MAX_KEYLEN) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("key size %"PRIuGRUB_SIZE" exceeds > > maximum %d bits"), > > + key_size * GRUB_CHAR_BIT, > > + GRUB_CRYPTODISK_MAX_KEYLEN * GRUB_CHAR_BIT); > > + > > + /* Check disk sector size */ > > + if (sector_size < GRUB_DISK_SECTOR_SIZE) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("sector size -S must be at least %d"), > > + GRUB_DISK_SECTOR_SIZE); > > + if ((sector_size & (sector_size - 1)) != 0) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, > > + N_("sector size -S %"PRIuGRUB_SIZE" is not > > power of 2"), > > + sector_size); > > + > > + /* Allocate all stuff here */ > > + hash = state[OPTION_HASH].set ? grub_strdup > > (state[OPTION_HASH].arg) : NULL; > > + cipher = grub_strdup (state[OPTION_CIPHER].arg); > > + keyfile = state[OPTION_KEYFILE].set ? > > + grub_strdup (state[OPTION_KEYFILE].arg) : NULL; > > + dev = grub_zalloc (sizeof *dev); > > + key_data = grub_zalloc (GRUB_CRYPTODISK_MAX_PASSPHRASE); > > + uuid = state[OPTION_UUID].set ? grub_strdup > > (state[OPTION_UUID].arg) : NULL; > > + if ((hash == NULL && state[OPTION_HASH].set) || cipher == NULL > > || dev == NULL || > > I think it would be more natural if you do "(state[OPTION_HASH].set > && hash == NULL)..." instead of "(hash == NULL && > state[OPTION_HASH].set)...". > > > + (keyfile == NULL && state[OPTION_KEYFILE].set) || key_data > > == NULL || > > Ditto. > > > + (uuid == NULL && state[OPTION_UUID].set)) > > Ditto. > > > + { > > + err = grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory"); > > + goto exit; > > + } > > + > > + /* Copy user password from -p option */ > > + if (state[OPTION_PASSWORD].set) > > + { > > + /* > > + * Password from the '-p' option is limited to C-string. > > + * Arbitrary data keys are supported via keyfiles. > > + */ > > + password_size = grub_strlen (state[OPTION_PASSWORD].arg); > > + grub_memcpy (key_data, state[OPTION_PASSWORD].arg, > > password_size); > > + } > > + > > + /* Copy user UUID from -u option */ > > + if (state[OPTION_UUID].set) > > + grub_memcpy (uuid, state[OPTION_UUID].arg, > > + grub_strlen (state[OPTION_UUID].arg)); > > This seems duplicate. grub_strdup() did work for you above. > > > + > > + /* Set cipher mode (tested above) */ > > + mode = grub_strchr (cipher,'-'); > > + *mode++ = '\0'; > > + > > + /* Check cipher */ > > + if (grub_cryptodisk_setcipher (dev, cipher, mode) != > > GRUB_ERR_NONE) > > + { > > + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid cipher > > %s"), cipher); > > + goto exit; > > + } > > + > > + /* Open SOURCE disk */ > > + diskname = args[0]; > > + len = grub_strlen (diskname); > > + if (len && diskname[0] == '(' && diskname[len - 1] == ')') > > + { > > + disklast = &diskname[len - 1]; > > + *disklast = '\0'; > > + diskname++; > > + } > > + disk = grub_disk_open (diskname); > > + if (disk == NULL) > > + { > > + if (disklast) > > + *disklast = ')'; > > + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("cannot open > > disk %s"), diskname); > > + goto exit; > > + } > > + > > + /* Get password from console */ > > + if (!state[OPTION_KEYFILE].set && key_data[0] == '\0') > > + { > > + char *part = grub_partition_get_name (disk->partition); > > + grub_printf_ (N_("Enter passphrase for %s%s%s: "), disk->name, > > + disk->partition != NULL ? "," : "", > > + part != NULL ? part : N_("UNKNOWN")); > > + grub_free (part); > > + > > + if (!grub_password_get ((char*)key_data, > > GRUB_CRYPTODISK_MAX_PASSPHRASE-1)) > > s/(char*)key_data/(char *) key_data/ > s/GRUB_CRYPTODISK_MAX_PASSPHRASE-1/GRUB_CRYPTODISK_MAX_PASSPHRASE - 1/ > > > + { > > + err = grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error reading > > password")); > > + goto exit; > > + } > > + /* > > + * Password from interactive console is limited to C-string. > > + * Arbitrary data keys are supported via keyfiles. > > + */ > > + password_size = grub_strlen (key_data); > > + } > > + > > + /* Warn if hash and keyfile are both provided */ > > + if (state[OPTION_KEYFILE].set && state[OPTION_HASH].arg) > > + grub_printf_ (N_("warning: hash is ignored if keyfile is > > specified\n")); + > > + /* Warn if -p option is specified with keyfile */ > > + if (state[OPTION_PASSWORD].set && state[OPTION_KEYFILE].set) > > + grub_printf_ (N_("warning: password specified with -p option " > > + "is ignored if keyfile is provided\n")); > > + > > + /* Warn of -O is provided without keyfile */ > > + if (state[OPTION_KEYFILE_OFFSET].set && > > !state[OPTION_KEYFILE].set) > > + grub_printf_ (N_("warning: keyfile offset option -O " > > + "specified without keyfile option -d\n")); > > + > > + grub_dprintf ("plainmount", "parameters: cipher=%s, hash=%s, > > key_size=%" > > + PRIuGRUB_SIZE", keyfile=%s, keyfile > > offset=%"PRIuGRUB_SIZE"\n", > > s/"PRIuGRUB_SIZE"/" PRIuGRUB_SIZE "/ > > > + cipher, hash, key_size, keyfile, keyfile_offset); > > + > > + err = plainmount_configure_sectors (dev, disk, sector_size); > > + if (err != GRUB_ERR_NONE) > > + goto exit; > > + > > + /* Configure keyfile or password */ > > + if (state[OPTION_KEYFILE].set) > > + err = plainmount_configure_keyfile (keyfile, key_data, > > key_size, keyfile_offset); > > + else > > + err = plainmount_configure_password (dev, hash, key_data, > > key_size, password_size); > > + if (err != GRUB_ERR_NONE) > > + goto exit; > > + > > + err = plainmount_setkey (dev, key_data, key_size); > > + if (err != GRUB_ERR_NONE) > > + goto exit; > > + > > + err = grub_cryptodisk_insert (dev, diskname, disk); > > + if (err != GRUB_ERR_NONE) > > + goto exit; > > + > > + dev->modname = "plainmount"; > > + dev->source_disk = disk; > > + plainmount_set_uuid (dev, uuid); > > + > > +exit: > > + grub_free (hash); > > + grub_free (cipher); > > + grub_free (keyfile); > > + grub_free (key_data); > > + grub_free (uuid); > > + if (err != GRUB_ERR_NONE && disk) > > s/disk/disk != NULL/ > > > + grub_disk_close (disk); > > + if (err != GRUB_ERR_NONE && dev) > > You can drop "dev" check from here. > > > + grub_free (dev); > > + return err; > > +} > > Daniel