Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption
On Thu, 2021-11-18 at 15:49 +0100, Daniel Kiper wrote: > Hey, > > Adding Denis, Patrick and Glenn... > > James, please add them to the loop next time. Sure ... is there some way of telling who should be cc'd (I'm not a fan of the kernel get_maintainer.pl but it gives you a list you can trim)? > > On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote: > > From: James Bottomley > > > > v3: make password getter specify prompt requirement. Update for > > TDX: > > Make name more generic and expand size of secret area > > > > > > https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba > > > > https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299 > > > > Redo the cryptodisk secret handler to make it completely > > generic > > and pluggable using a list of named secret providers. Also > > allow an optional additional argument for secret providers that may > > have more than one secret. > > > > v2: update geli.c to use conditional prompt and add callback for > > variable message printing and secret destruction > > > > To achieve encrypted disk images in the AMD SEV and other > > confidential computing encrypted virtual machines, we need to add > > the ability for grub to retrieve the disk passphrase from an OVMF > > provisioned > > configuration table. > > > > https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c > > > > The patches in this series modify grub to look for the disk > > passphrase in the secret configuration table and use it to decrypt > > any disks in the system if they are found. This is so an encrypted > > image with a properly injected password will boot without any user > > intervention. > > > > The three patches firstly modify the cryptodisk consumers to allow > > arbitrary password getters instead of the current console based > > one. The next patch adds a '-s module [id]' option to cryptodisk > > to allow it to use plugin provided passwords and the final one adds > > a sevsecret command to check for the secrets configuration table > > and provision the disk passphrase from it if an entry is > > found. With all this in place, the sequence to boot an encrypted > > volume without user intervention is: > > > > cryptomount -s efisecret > > source (crypto0)/boot/grub.cfg > > > > Assuming there's a standard Linux root partition. > > Thank you for posting this patch series. Unfortunately it conflicts > with [1] patches. And I want to take [1] first because it is an > important improvement for GRUB's crypto infrastructure. Additionally, > as Glenn said in [1], this crypto infra change should simplify your > code too. > > I have just finished reviewing Glenn's patches and waiting for v4. > I hope we will be able to merge it soon. If you could take a look at > the [1] and check if it does not make any troubles for you it would > be perfect. > > I will drop you a line when Glenn's patches are in the tree and you > can rebase your patch set on top of it. Yes, the rebase looks trivial. I'll do it and repost as soon as the patches are upstream. Regards, James ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption
Hey, Adding Denis, Patrick and Glenn... James, please add them to the loop next time. On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote: > From: James Bottomley > > v3: make password getter specify prompt requirement. Update for TDX: > Make name more generic and expand size of secret area > > > https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba > > https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299 > > Redo the cryptodisk secret handler to make it completely generic > and pluggable using a list of named secret providers. Also allow > an optional additional argument for secret providers that may have > more than one secret. > > v2: update geli.c to use conditional prompt and add callback for > variable message printing and secret destruction > > To achieve encrypted disk images in the AMD SEV and other confidential > computing encrypted virtual machines, we need to add the ability for > grub to retrieve the disk passphrase from an OVMF provisioned > configuration table. > > https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c > > The patches in this series modify grub to look for the disk passphrase > in the secret configuration table and use it to decrypt any disks in > the system if they are found. This is so an encrypted image with a > properly injected password will boot without any user intervention. > > The three patches firstly modify the cryptodisk consumers to allow > arbitrary password getters instead of the current console based one. > The next patch adds a '-s module [id]' option to cryptodisk to allow > it to use plugin provided passwords and the final one adds a sevsecret > command to check for the secrets configuration table and provision the > disk passphrase from it if an entry is found. With all this in place, > the sequence to boot an encrypted volume without user intervention is: > > cryptomount -s efisecret > source (crypto0)/boot/grub.cfg > > Assuming there's a standard Linux root partition. Thank you for posting this patch series. Unfortunately it conflicts with [1] patches. And I want to take [1] first because it is an important improvement for GRUB's crypto infrastructure. Additionally, as Glenn said in [1], this crypto infra change should simplify your code too. I have just finished reviewing Glenn's patches and waiting for v4. I hope we will be able to merge it soon. If you could take a look at the [1] and check if it does not make any troubles for you it would be perfect. I will drop you a line when Glenn's patches are in the tree and you can rebase your patch set on top of it. Daniel [1] https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00107.html ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args
On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> The member found_uuid was never used by the crypto-backends, but was used to
Ha! Could you make this patch second in this patch series? Then we could
avoid carrying over have_it/found_uuid cruft over succeeding patches.
> 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
> iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
s/iff/if/
> 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 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
I like if you add "()" to function names in comments, etc.
> grub_cryptodisk_scan_device when a crypto device is successfully decrypted
I think one "when" is redundant here. Or something else is wrong.
> or when the source device has already been successfully opened. Prior to
> this change, trying to mount an already successfully opened device would
> trigger an error with the message "no such cryptodisk found", which is at
> best misleading. The mount should silently succeed in this case, which is
> what happens with this patch.
>
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 9 -
> include/grub/cryptodisk.h | 1 -
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 5b38606ed..8e5277314 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
>
> grub_cryptodisk_insert (dev, name, source);
>
> -cargs->found_uuid = 1;
> -
> goto cleanup;
>}
>goto cleanup;
> @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
>
>if (err)
> grub_print_error ();
> - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> + return (!err && cargs->search_uuid) ? 1 : 0;
err == GRUB_ERR_NONE && cargs->search_uuid != NULL
> }
>
> static grub_err_t
> @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>
>if (state[0].set) /* uuid */
> {
> + int found_uuid = 0;
Zero initialization seems redundant here.
>grub_cryptodisk_t dev;
>
>dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>
>cargs.check_boot = state[2].set;
>cargs.search_uuid = args[0];
> - grub_device_iterate (_cryptodisk_scan_device, );
> + found_uuid = grub_device_iterate (_cryptodisk_scan_device,
> );
>
> - if (!cargs.found_uuid)
> + if (!found_uuid)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>return GRUB_ERR_NONE;
> }
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/cryptodisk.c | 26 +-
> grub-core/disk/geli.c | 9 -
> grub-core/disk/luks.c | 11 +--
> grub-core/disk/luks2.c | 6 +++---
> include/grub/cryptodisk.h | 10 --
> 5 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index a5f7b860c..5b38606ed 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, have_it;
> -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 grub_errno;
> if (!dev)
> @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>
> grub_cryptodisk_insert (dev, name, source);
>
> -have_it = 1;
> +cargs->found_uuid = 1;
Please say in the commit message you are changing variable/member name too.
Or maybe it would be better if you do this in separate patch.
> goto cleanup;
>}
> @@ -1091,7 +1088,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, NULL);
> if (grub_errno)
>return grub_errno;
> if (!dev)
> @@ -1135,7 +1132,7 @@ grub_cryptodisk_scan_device (const char *name,
>
>if (err)
> grub_print_error ();
> - return have_it && search_uuid ? 1 : 0;
> + return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
I think this should be enough:
return (cargs->found_uuid && cargs->search_uuid != NULL)
> }
>
> static grub_err_t
> @@ -1153,7 +1150,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>cargs.key_len = grub_strlen (state[3].arg);
> }
>
> - have_it = 0;
cargs->found_uuid = 0?
>if (state[0].set) /* uuid */
> {
>grub_cryptodisk_t dev;
> @@ -1166,21 +1162,18 @@ 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];
>grub_device_iterate (_cryptodisk_scan_device, );
> - search_uuid = NULL;
cargs.search_uuid = NULL?
> - if (!have_it)
> + if (!cargs.found_uuid)
> 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)) /* -a|-b */
> {
> - search_uuid = NULL;
> - check_boot = state[2].set;
> + cargs.check_boot = state[2].set;
>grub_device_iterate (_cryptodisk_scan_device, );
> - search_uuid = NULL;
Ditto. If this is correct then I think it should be shortly explained in
the commit message why you can drop these assignments.
>return GRUB_ERR_NONE;
> }
>else
> @@ -1192,8 +1185,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>char *disklast = NULL;
>grub_size_t len;
>
> - search_uuid = NULL;
Ditto.
> - 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 32f34d5c3..32d35521b 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 && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> {
> - grub_dprintf ("geli", "%s !=
