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

[Maxim Fomin]

------- 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
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
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?
Or I am missing something?

Best regards,
Maxim Fomin