[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto m
From: |
Patrick Steinhardt |
Subject: |
Re: [PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto modules |
Date: |
Sun, 14 Nov 2021 10:57:09 +0100 |
On Tue, Oct 12, 2021 at 06:26:25PM -0500, Glenn Washburn wrote:
> Updates since v2:
> * Add space after function name in function calls
> * Add comments for members of struct grub_cryptomount_args
> * Correct commit message for patch #4
>
> ---
> 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).
>
> The infrastructure is implemented in patch #1 along with adding a new -p
> parameter to cryptomount partly as an example to show how a password would be
> passed to the crypto module backends. The backends do nothing with this data
> in this patch, but print a message saying that sending a password is
> unimplemented.
>
> Patch #2 takes advantage of this new data passing mechanism to refactor the
> essentially duplicated code in each crypto backend module for inputting the
> password and puts that functionality in the cryptodisk code. Conceptually,
> the crypto backends should not be getting user input anyway.
>
> Patch #3 gets rid of some long time globals in cryptodisk, moving them
> into the passed struct.
>
> Patch #4 removes the found_uuid flag from the cargs struct, which is not
> needed because the same information can be obtained from the return value
> of grub_device_iterate.
>
> My intention is for this patch series to lay the foundation for an improved
> patch series providing detached header and keyfile support (I already have
> the series updated and ready to send once this is accepted). I also believe
> tha this will somewhat simplify the patch series by James Bottomley in
> passing secrets to the crypto backends.
>
> Glenn
A single question for patch 3/4, but all the others look good to me.
Feel free to add "Reviewed-by: Patrick Steinhardt <ps@pks.im>" to those.
Patrick
> Glenn Washburn (4):
> cryptodisk: Add infrastructure to pass data from cryptomount to
> cryptodisk modules
> cryptodisk: Refactor password input out of crypto dev modules into
> cryptodisk
> cryptodisk: Move global variables into grub_cryptomount_args struct
> cryptodisk: Remove unneeded found_uuid from cryptomount args
>
> grub-core/disk/cryptodisk.c | 108 ++++++++++++++++++++++++------------
> grub-core/disk/geli.c | 35 ++++--------
> grub-core/disk/luks.c | 37 ++++--------
> grub-core/disk/luks2.c | 33 ++++-------
> include/grub/cryptodisk.h | 19 ++++++-
> 5 files changed, 120 insertions(+), 112 deletions(-)
>
> Range-diff against v2:
> 1: 475bf7e9e ! 1: 83112518f cryptodisk: Add infrastructure to pass data
> from cryptomount to cryptodisk modules
> @@ grub-core/disk/cryptodisk.c: static grub_err_t
> + if (state[3].set) /* password */
> + {
> + cargs.key_data = (grub_uint8_t *) state[3].arg;
> -+ cargs.key_len = grub_strlen(state[3].arg);
> ++ cargs.key_len = grub_strlen (state[3].arg);
> + }
> +
> have_it = 0;
> 2: a0c0694fc ! 2: 65a18c5e8 cryptodisk: Refactor password input out of
> crypto dev modules into cryptodisk
> @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
> char *name,
> + ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not
> supplied");
> + goto error;
> + }
> -+ cargs->key_len = grub_strlen((char *) cargs->key_data);
> ++ cargs->key_len = grub_strlen ((char *) cargs->key_data);
> + }
> +
> + ret = cr->recover_key (source, dev, cargs);
> @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
> char *name,
> + if (askpass)
> + {
> + cargs->key_len = 0;
> -+ grub_free(cargs->key_data);
> ++ grub_free (cargs->key_data);
> + }
> + return ret;
> }
> 3: 419f114d8 ! 3: fed38b3dc cryptodisk: Move global variables into
> grub_cryptomount_args struct
> @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char
> *name,
>
> static grub_err_t
> @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
> (grub_extcmd_context_t ctxt, int argc, char **args)
> - cargs.key_len = grub_strlen(state[3].arg);
> + cargs.key_len = grub_strlen (state[3].arg);
> }
>
> - have_it = 0;
> @@ include/grub/cryptodisk.h: 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;
> + grub_uint32_t found_uuid : 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;
> @@ include/grub/cryptodisk.h: struct grub_cryptodisk_dev
> struct grub_cryptodisk_dev *next;
> struct grub_cryptodisk_dev **prev;
> 4: e6e1e8bc3 ! 4: 854709929 cryptodisk: Remove unneeded found_uuid from
> cryptomount args
> @@ Commit message
> iff grub_cryptodisk_scan_device returns 1. And
> grub_cryptodisk_scan_device
> will only return 1 if a search_uuid has been specified and a
> cryptodisk was
> successfully setup by a crypto-backend. So the return value of
> - grub_cryptodisk_scan_device is equivalent to found_uuid.
> + grub_cryptodisk_scan_device is almost equivalent to found_uuid, with
> the
> + exception of the case where a mount is requested or an already opened
> + crypto device.
> +
> + With this change grub_device_iterate will return 1 when
> + grub_cryptodisk_scan_device 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.
>
> ## grub-core/disk/cryptodisk.c ##
> @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
> char *name,
> @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
> (grub_extcmd_context_t ctxt, i
> }
>
> ## include/grub/cryptodisk.h ##
> -@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
> - struct grub_cryptomount_args
> +@@ include/grub/cryptodisk.h: struct grub_cryptomount_args
> {
> + /* scan: Flag to indicate that only bootable volumes should be
> decrypted */
> grub_uint32_t check_boot : 1;
> - grub_uint32_t found_uuid : 1;
> + /* scan: Only volumes matching this UUID should be decrpyted */
> char *search_uuid;
> - grub_uint8_t *key_data;
> - grub_size_t key_len;
> + /* recover_key: Key data used to decrypt voume */
> --
> 2.27.0
>
signature.asc
Description: PGP signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto modules,
Patrick Steinhardt <=