On Sun, 3 Oct 2021 22:22:40 +0200 Patrick Steinhardt <p...@pks.im> wrote:
> On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote: > > The member found_uuid was never used by the crypto-backends, but was used to > > 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 > > 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. > > Is that always the case though? Most notably, `scan_device_real` will > only set `have_it = 1` in case we have recovered the key. If the > cryptodisk existed before and was found via `get_by_source_disk`, then > we wouldn't set `have_it` at all. This essentially means that we'd only > set `have_it` in case we found a new cryptodisk, but not if we return a > preexisting one. > > I don't know whether this behaviour is something we rely on. My gut > feeling says it's rather a bug in the current code, which seems to be > confirmed by the error message in the `if (!have_it)` case, which says > "no such cryptodisk found". We did find one, but it's not a new one. > > So in total I think your patch makes sense and fixes a bug, but the > description doesn't quite match reality. Good catch, I actually hadn't thought about this case, and inadvertently fixed this bug. I'll update the commit message to reflect this. I don't believe the behavior of this "bug" is relied on anywhere either. Glenn > > Patrick > > > Signed-off-by: Glenn Washburn <developm...@efficientek.com> > > --- > > 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 5e153ee0a..033894257 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; > > } > > > > 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; > > 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 (&grub_cryptodisk_scan_device, &cargs); > > + found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, > > &cargs); > > > > - if (!cargs.found_uuid) > > + if (!found_uuid) > > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found"); > > return GRUB_ERR_NONE; > > } > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > index 230167ab0..f4afb9cbd 100644 > > --- a/include/grub/cryptodisk.h > > +++ b/include/grub/cryptodisk.h > > @@ -70,7 +70,6 @@ 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; > > -- > > 2.32.0 > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel