On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote: > On Sun, 12 Sep 2021 13:17:29 +0200 > Patrick Steinhardt wrote: > > > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote: > > > On Mon, 30 Aug 2021 20:02:26 +0200 > > > Patrick Steinhardt wrote: > > > > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote: > > > > > Signed-off-by: Glenn Washburn > > > > > --- > > > > > grub-core/disk/cryptodisk.c | 26 +++++++++----------------- > > > > > include/grub/cryptodisk.h | 3 +++ > > > > > 2 files changed, 12 insertions(+), 17 deletions(-) > > > > > > > > > > diff --git a/grub-core/disk/cryptodisk.c > > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 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->search_uuid, cargs->check_boot); > > > > > if (grub_errno) > > > > > return grub_errno; > > > > > if (!dev) > > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char > > > > > *name, > > > > > grub_cryptodisk_insert (dev, name, source); > > > > > > > > > > - have_it = 1; > > > > > + cargs->found_uuid = 1; > > > > > > > > > > goto cleanup; > > > > > } > > > > > @@ -1093,7 +1090,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, 0); > > > > > if (grub_errno) > > > > > return grub_errno; > > > > > if (!dev) > > > > > @@ -1137,7 +1134,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; > > > > > } > > > > > > > > > > static grub_err_t > > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t > > > > > ctxt, int argc, char **args) cargs.key_len = > > > > > grub_strlen(state[3].arg); } > > > > > > > > > > - have_it = 0; > > > > > if (state[0].set) /* uuid */ > > > > > { > > > > > grub_cryptodisk_t dev; > > > > > @@ -1168,21 +1164,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 (&grub_cryptodisk_scan_device, &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 (&grub_cryptodisk_scan_device, &cargs); > > > > > - search_uuid = NULL; > > > > > return GRUB_ERR_NONE; > > > > > } > > > > > else > > > > > @@ -1194,8 +1187,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/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > > > > index 1070140d9..11062f43a 100644 > > > > > --- a/include/grub/cryptodisk.h > > > > > +++ b/include/grub/cryptodisk.h > > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t > > > > > > > > > > struct grub_cryptomount_args > > > > > { > > > > > + grub_uint32_t check_boot : 1; > > > > > + grub_uint32_t found_uuid : 1; > > > > > + char *search_uuid; > > > > > grub_uint8_t *key_data; > > > > > grub_size_t key_len; > > > > > }; > > > > > > > > Aren't these parameters in a different scope than the key data? These > > > > are only used for device discovery via `scan()`, while the other ones > > > > are for decrypting the key. Do we want to split those up into two > > > > different structs? > > > > > > This struct is meant to be used for any data passed to the crypto > > > backend from cryptomount. All of those members are affected by > > > cryptomount options. So this struct isn't about anything in particular, > > > just a common set of data passed to the crypto backends via > > > cryptomount. So I don't think two structs would improve anything here. > > > Am I missing something? > > > > > > Glenn > > > > I'm mostly wondering about lifetimes of these parameters. They are used > > in different phases of the cryptomount: some are used only at the time > > of discovery, while others are used at decryption time, where it's not > > immediately clear which parameters are used when without having a look > > at the cryptodisk implementations. That's why I was thinking it might > > make more sense to split them up by those phases such that this becomes > > explicit. > > > > Patrick > > Okay, I think I see what you're saying. Essentially, as someone wanting > to write a new crypto-backend, when I'm writing by scan function, how > do I know what fields in the cargs struct are valid (have been > assigned)? The answer would be to check if they are not NULL, and > otherwise they're available. I don't really see an issue. > > Would your objection be alleviated by having cargs be redefined like so: > > struct grub_cryptomount_args > { > struct { > grub_uint32_t check_boot : 1; > char *search_uuid; > } scan; > struct { > grub_uint8_t *key_data; > grub_size_t key_len; > } recover_key; > struct { > ... > } common; > }; > > Then in scan you know you can only use scan and common structs. I have > a common because the detached header changes will be such that scan and > recover_key need to be provided with detached header. I think this way > is more than is needed at the present moment. If I'm still not getting > it, can you be a little more concrete in what is problematic and how > you think it could be changed to be better? > > Glenn Sorry, took me quite some time to get to this. I like your proposed approach, and if we sprinkle in some comments about when those structs should be used then I think it would be easy enough to understand. It does raise the question whether we should instead define `grub_cryptomount_args_common` and then embed it in `grub_cryptomount_args_{scan,recover_key}` structs such that it is impossible to use args in the wrong phase (e.g. `recover_key` args during scan). But I feels a bit like bikeshedding, so please feel free to go with your preferred style. Patrick