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

[Maxim Fomin]

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

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

Best regards,
Maxim Fomin