Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
--- Original Message --- On Sunday, June 26th, 2022 at 9:20 PM, Glenn Washburn wrote: > > > On Sun, 26 Jun 2022 13:37:07 + > Maxim Fomin ma...@fomin.one wrote: > > > --- 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 > > > 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_conf
Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
On Tue, 28 Jun 2022 05:07:43 + Maxim Fomin wrote: > --- Original Message --- > On Sunday, June 26th, 2022 at 9:20 PM, Glenn Washburn > wrote: > > > > > > On Sun, 26 Jun 2022 13:37:07 + > > Maxim Fomin ma...@fomin.one wrote: > > > > > --- 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 > > > > > > 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()' ind
Re: [PATCH v3 1/1] plainmount: Support plain encryption mode.
--- Original Message --- On Tuesday, June 28th, 2022 at 5:46 PM, Glenn Washburn wrote: > 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. Agreed. > 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'. cryptsetup supports only 512 byte block alignment for LUKS1 and 512-4096 alignment for LUKS2. Its offset parameter is specified in terms of the number of 512 byte blocks. > > 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 > I was not saying about some new syntax for some device offset argument. I am speaking about this: loopback node (hd0,gpt2)2048+ plainmount node-c aes-xts-plain64 -h sha512 // works plainmount (hd0,gpt2)2048+ -c aes-xts-plain64 -h sha512 // currently - no Currently the second command does not work without loopback, I am thinking about removing this limitation in plainmount. Best regards, Maxim Fomin ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
