grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/4] cryptodisk: Add options to cryptomount to support keyfil


From: Daniel Kiper
Subject: Re: [PATCH 3/4] cryptodisk: Add options to cryptomount to support keyfiles
Date: Fri, 13 May 2022 13:12:35 +0200
User-agent: NeoMutt/20170113 (1.7.2)

On Thu, May 12, 2022 at 01:53:31PM -0500, Glenn Washburn wrote:
> On Thu, 12 May 2022 19:45:48 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Fri, May 06, 2022 at 03:45:59AM -0500, Glenn Washburn wrote:
> > > From: John Lane <john@lane.uk.net>
> > >
> > > Add the options --key-file, --keyfile-offset, and --keyfile-size to
> > > cryptomount and code to put read the requested key file data and pass
> > > via the cargs struct. Note, key file data is for all intents and purposes
> > > equivalent to a password given to cryptomount. So there is no need to
> > > enable support for key files in the various crypto backends (eg. LUKS1)
> > > because the key data is passed just as if it were a password.
> > >
> > > Signed-off-by: John Lane <john@lane.uk.net>
> > > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit 
> > > message
> > > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > > development@efficientek.com: rebase and rework to use cryptomount arg 
> > > passing,
> > >   minor fixes, improve commit message
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c | 86 ++++++++++++++++++++++++++++++++++++-
> > >  include/grub/cryptodisk.h   |  2 +
> > >  include/grub/file.h         |  2 +
> > >  3 files changed, 89 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 9f5dc7acb..19af4fa49 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -42,6 +42,9 @@ static const struct grub_arg_option options[] =
> > >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> > >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 
> > > 0},
> > >      {"password", 'p', 0, N_("Password to open volumes."), 0, 
> > > ARG_TYPE_STRING},
> > > +    {"key-file", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
> > > +    {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, 
> > > ARG_TYPE_INT},
> > > +    {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
> > > ARG_TYPE_INT},
> > >      {0, 0, 0, 0, 0, 0}
> > >    };
> > >
> > > @@ -1172,6 +1175,85 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > > int argc, char **args)
> > >        cargs.key_len = grub_strlen (state[3].arg);
> > >      }
> > >
> > > +  if (state[4].set) /* keyfile */
> > > +    {
> > > +      const char *p = NULL;
> > > +      grub_file_t keyfile;
> > > +      unsigned long long keyfile_offset = 0, keyfile_size = 0;

I think keyfile_size should be "unsigned long" if you use grub_strtoul() below.

> > > +
> > > +      if (state[5].set) /* keyfile-offset */
> > > + {
> > > +   keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> >
> > Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?
>
> I was thinking the allowing the largest possible offset might be useful
> to allow for using unallocated space at the end of large block devices.
> This could still be done with smaller offsets, by using the blocklist
> syntax to create a "file" that starts near the end and then use a much
> smaller offset. Why don't you like it as is?

I would not say I do not like it. I just want to understand discrepancy
between this conversion and one below. If you need "unsigned long long"
go ahead with it. Though I would suggest to check the value does not
exceed maximum value of target type. I think GRUB_TYPE_U_MAX() can be
useful here. Same applies to conversion below.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]