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

[Glenn Washburn <development@efficientek.com>]

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?

> 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.

> 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.

Glenn

> Or I am missing something?
> 
> Best regards,
> Maxim Fomin