------- Original Message ------- On Saturday, June 25th, 2022 at 12:55 AM, Glenn Washburn <developm...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
