[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomou
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct |
Date: |
Wed, 1 Dec 2021 16:07:39 -0600 |
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
- Re: [PATCH v3 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct,
Glenn Washburn <=