[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: |
Glenn Washburn |
Subject: |
Re: [PATCH 3/4] cryptodisk: Add options to cryptomount to support keyfiles |
Date: |
Fri, 13 May 2022 11:39:48 -0500 |
On Fri, 13 May 2022 13:12:35 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:
> 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.
I made them both unsigned long long because I wanted both using
grub_strtoull().
> > > > +
> > > > + 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.
Ahh, now I see what you're getting at. I hadn't noticed the difference.
I meant to change the other one to grub_strtoull, but forgot. I'll do
that.
Glenn
[PATCH 4/4] docs: Add documentation on keyfile option to cryptomount, Glenn Washburn, 2022/05/06