Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.

[Glenn Washburn]

On Tue, 28 Jun 2022 05:07:43 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> ------- Original Message -------
> On Sunday, June 26th, 2022 at 9:20 PM, Glenn Washburn 
> <development@efficientek.com> wrote:
> >
> >
> > On Sun, 26 Jun 2022 13:37:07 +0000
> > Maxim Fomin maxim@fomin.one wrote:
> >
> > > ------- Original Message -------
> > > On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn 
> > > development@efficientek.com wrote:
> > >
> > > > Hmm, I wasn't suggesting this be added. I hope you didn't think I was
> > > > suggesting this. What I was suggesting was that the block list syntax
> > > > already supported in GRUB for device paths be used, not creating a new
> > > > block list syntax just for this command. You shouldn't need to add any
> > > > new code for what I was suggesting.
> > > >
> > > > For instance, if you know that your plain mount volume is on device
> > > > (hd0) at offset 1M and have a keyfile at (hd1)/path/to/keyfile where
> > > > the key material is offset 35235 bytes into that file you would use:
> > > >
> > > > loopback cplain0 (hd0)2048+
> > > > plainmount -c ... -s ... -d (hd1)/path/to/keyfile -O 35235 (cplain0)
> > > >
> > > > If the keyfile data is on disk (hd1) at offset 16708 (16*1024 + 324),
> > > > then use:
> > > >
> > > > plainmount -c ... -s ... -d (hd1)32+ -O 324 (cplain0)
> > > >
> > > > or
> > > >
> > > > plainmount -c ... -s ... -d (hd1)+ -O 16708 (cplain0)
> > > >
> > > > Here the '+' is needed after (hd1) to turn it into a file because -d
> > > > should only take a file. It would be nice to have (hd1) be treated as
> > > > (hd1)+ when used as a file, but that would be a different patch.
> > > >
> > > > The drawback to what I'm suggesting is that you can't do "-d
> > > > (hd1)16K+". This could be something interesting to add to GRUB
> > > > blocklist syntax, but as a separate patch.
> > > >
> > > > I believe there's also a confusion here on the usage of blocklist
> > > > syntax. Blocklist syntax is about specifying a range of blocks, not an
> > > > offset or specific block number. So for instance, "(hd1)+16" means
> > > > blocks 0-15, a total of 8K bytes, so bytes past 8K are unreadable. On
> > > > the other hand, "(hd1)16+" means blocks 16 to end of device. I think the
> > > > latter is what you want.
> > >
> > > ...
> > >
> > > > > +/ Read keyfile as a disk segment */
> > > > > +static grub_err_t
> > > > > +plainmount_configure_keydisk (grub_cryptodisk_t dev, char *keyfile, 
> > > > > grub_uint8_t *key_data,
> > > > > + grub_size_t key_size, grub_size_t keyfile_offset)
> > > >
> > > > I don't think this function should exist either. Using GRUB's already
> > > > existing blocklist syntax (see example above) and with -O for
> > > > specifying keyfile offset, we don't need this.
> > >
> > > ...
> > >
> > > > > + / Configure keyfile/keydisk/password */
> > > > > + if (cargs.keyfile)
> > > > > + if (cargs.keyfile[0] == '/' ||
> > > > > + (grub_strchr (cargs.keyfile, ')') < 
> > > > > grub_strrchr(cargs.keyfile,'/')))
> > > > > + err = plainmount_configure_keyfile (dev, cargs.keyfile, 
> > > > > cargs.key_data,
> > > > > + cargs.key_size, cargs.keyfile_offset);
> > > > > + else
> > > > > + err = plainmount_configure_keydisk (dev, cargs.keyfile, 
> > > > > cargs.key_data,
> > > > > + cargs.key_size, cargs.keyfile_offset);
> > > >
> > > > We shouldn't support sending a device as a keyfile and only support
> > > > files. As noted above, if the keyfile data is only accessibly via some
> > > > blocks on a disk device, then use the builtin blocklist syntax
> > > > potentially with the -O keyfile offset.
> > > >
> > > > Glenn
> > >
> > > I don't quite understand this. Irrespective of how device argument is 
> > > sent (and syntax used),
> > > processing device blocks in 'configure_keyfile()' differes from 
> > > processing a file. I tested
> >
> >
> > This isn't making sense to me. The function
> > plainmount_configure_keyfile(), which I presume you are referring to
> > above, uses grub_file_open(), so it expects a file-type argument (which
> > is a (dev)/path/to/file path or (dev)N+M blocklist). How does this
> > differ from processing a file?
> 
> I wanted to say 'configure_keydisk' instead of 'configure_keyfile'. But the 
> comment below shows
> you understood my point.
> 
> > > grub_file_open() on a loopback device and it does not work. It makes 
> > > sense, because neither
> > > '(hdx,gpty)NNN+' nor a loopback node on top of it is a file. So, I think 
> > > that supporting
> >
> >
> > Yes, grub_file_open() does not open raw devices (although I think it
> > should). However, you also seem to say that '(hdx,gpty)NNN+' is not a
> > file, which I take to mean that it can not be opened by
> > grub_file_open(). But look at the source for grub_file_open() in
> > grub-core/kern/file.c (search for the comment with the word
> > "blocklist"). There you will find that grub_file_open does open
> > blocklists, so blocklists can be used where file paths are used.
> 
> After rechecking this issue it seems 'grub_file_open()' indeed supports 
> blocklist syntax.
> 
> > > blocks on disk requires some additional code in 'configure_keyfile()'. 
> > > Perhaps you mean moving
> > > 'configure_keydisk()' code inside 'plainmount_configure_keyfile()' and 
> > > removing it definition?
> >
> >
> > Nope, I'm saying to get rid of plainmount_configure_keydisk()
> > completely. I haven't precisely tested this case, so I'm not 100%
> > certain of the above, but I'm over 90% certain that its true.
> >
> > For instance, note that the cat command uses grub_file_open() and the
> > following works: cat (dev)+1.
> 
> I will completely remove 'configure_keydisk' as it is not necessary.
> 
> One more point - are you sure the '-O' option mentioned in the previous email 
> is really needed if
> the keyfile offset can be specified with the blocklist syntax? It looks 
> strange not to have '-o' option for
> encrypted device (relying on loopback file with blocklist syntax) but having 
> '-O' option for keyfile offset
> (which can be specified with blocklist syntax too - in this case even without 
> loopback).

The reason that we need '-O', or keyfile offset, is because block
syntax is for specifying block ranges, not byte ranges. Blocks are
sized in the native GRUB block size, which is 512 bytes. If we do not
have '-O', then keyfile data must be aligned on 512-byte boundaries,
which I think is an unreasonable restriction.

On the other hand, I don't think that a '-o' option is needed because
restricting plainmount encrypted data to be 512-byte aligned seems a
reasonable restriction. If the data is not 512-byte aligned, I suspect
you're going to get poorer read/write performance. This is maybe not a
problem for small plainmounts that may contain key material for
unlocking other volumes. So I'm open to this being a desirable feature,
and not dead-set against it.

It would be interesting to verify that cryptsetup cannot create a
LUKS1/LUKS2 volume where the data offset is not 512-byte aligned. I
think this is true, but the LUKS2 standard has the offset in bytes, so
its technically possible. If cryptsetup can create a volume where the
encrypted data is not 512-aligned, then there's a good case for adding
'-o'.

> Also, I am thinking whether it will be easier from the user perspective to 
> support blocklist syntax (without
> loopback) for device argument too - having the same syntax for device and 
> offset arguments is more clear.

Do you mean "file argument" instead of "device argument" here? Because
devices already support blocklist syntax.

> However, it will works only if 'grub_file_t' provides interface to 
> 'grub_disk_t' object which is needed for
> cryptodisk to read encrypted data. I didn't look at it, but if it works, the 
> command syntax can be simplified
> to blocklist syntax for both device and offset argument.

I'm not sure I'm understanding this. Can you give some examples of what
the new command syntax that you're thinking of might look like? How
would blocklist syntax be used for the offset argument? That sounds
like it could be confusing.

Glenn

> 
> Best regards,
> Maxim Fomin
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel