Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

On Sun, 14 Nov 2021 10:56:15 +0100
Patrick Steinhardt <ps@pks.im> wrote:

> On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  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;
> >  
> >      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 I didn't get this wrong, then all scan implementations
> unconditionally dereference the `grub_cryptomount_args_t` pointer.
> So why does this work, and shouldn't we pass down a struct which has the
> `search_uuid` and `check_boot` parameters properly set up?

I'm not sure that this does work, good catch. I don't have a setup to
test GRUB utils in conjunction with cryptodisk. If you do (or anyone
else does), testing would be greatly appreciated. Regardless, I believe
this is an issue.

My reading of the previous code is that search_uuid and check_boot are
not explicitly initialized here. The only reasonable conclusion I come
to is that they are initialized to zero by default by the compiler. So
I'll create a cargs struct with all fields initalized to zero and pass
that in. Does this sound good?

Glenn