------- Original Message -------
On Sunday, July 10th, 2022 at 9:07 PM, Glenn Washburn
<developm...@efficientek.com> wrote:
> > +
> > +plainmount hd0,gpt1 -o 1048576
> > +
> > +
> > +both create virtual devices with 1MiB offset on top of the specified
> > partition. The
> > +option @option{-o} is useful to specify offset which is not aligned to 512
> > byte
>
>
> I just noticed that the code takes this parameter and turns it into a
> number of sectors that the data is offset from the start of the device.
> Thus internally this gets rounded down to be sector aligned anyway. In
> fact, its aligned to the size of the sector specified with the -S
> option. So we should get rid of the -o entirely. In fact, -o will not
> work if it is not a multiple of the sector size specified with -S.
Option -o is taken as a byte number, not as a sector number, it is not
multiplied
by 512 or by sector size. However later, indeed it is truncated to a sector
size,
loosing its meaning. I agree with the rest of the comment, this option is
irrelevant
and should be removed.
>
> > +sector size. Note: current cryptsetup implementation of plain mode and
> > LUKS v1 restricts
> > +alignment to 512 byte sector size. Specifying arbitrary byte alignment is
> > useful only to
> > +open LUKS v2 volume if master key is known or to open the volume encrypted
> > by other
> > +cryptographic implementation. Note: in LUKS revealing master key is not
> > recommended
> > +because it allows to open encrypted device directly bypassing the header
> > and LUKS
> > +security features.
>
>
> These "Notes" should probably be reorganized as @footnote{Put note
> inside of this.} You have two notes here, so perhaps two foot notes,
> although it looks like here you have a nested note. I'm not sure if you
> can have nested footnotes.
I can change them to be just a single note.
> > +
> > +Password can be specified in two ways: as a password data from a keyfile
> > or as a secret
> > +passphrase. The keyfile parameter @option{-d} specifies path to the
> > keyfile containing
> > +password data and has higher priority than the other password method.
> > Specified password
> > +data in this mode is not hashed. The option @option{-O} specifies offset
> > in terms of bytes
>
>
> Hmm its not hashed? I thought a keyfile that contains 4 characters
> "pass" would be the same as entering "pass" as the secret passphrase.
> If the keyfile is not hashed, then the derived master key is different.
> Are you Am I mistaken or missing something?
I have taken this behavior from cryptsetup. Having binary key file data
directly allows
to have arbitrary password (not necessarily typable with a keyboard). Such
feature is not
very popular, but it exists. This feature is connected with use cases where
user types
password in non-English keyboard (possibly, a letter which is not easily
encoded in UTF,
or which encoding depends on keyboard layout), which cannot be typed at another
computer.
> > +of the password data from the start of the keyfile. This option has no
> > effect without the
> > +option @option{-d}. Offset of password data in the keyfile can be
> > specified directly with
> > +option @option{-d} and GRUB blocklist syntax (for example: '-d
> > (hd0,gpt1)2048+').
>
>
> Should this be -O instead of -d?
Why? The last part of quoted text says that alternative to option -O is the
blocklist syntax
which is part of the -d option.
> > +configured when creating the encrypted volume. Attempting to decrypt such
> > volume with
> > +different sector size than it was created with will not result in an
> > error, but will
> > +decrypt to random bytes and thus prevent accessing the volume.
>
>
> Actually, some sectors will be decrypted correctly. So perhaps we
> should say "but not all sectors will be properly decrypted, generally
> causing the volume to be inaccessible"
I would not complicate explanation. The difference between decrypted in such
'strange'
scenario FS and normal FS is so large, that no fs will work. For example, I
have a BTRFS
on top of -S 4096 encrypted partition. Once on live system (not GRUB) I forgot
to type
-S 4096 and received internal BTRFS error in dmesg saying something about bad
internal
metadata state and then general mount error. So yes, BTRFS can recognize that
this is a
BTRFS partition, but will refuse to mount. In GRUB mode such device (wrong
sector size)
is not recognized too. I think what can be added here is at most a line saying
that FS
may ocassionally detect underlying FS, but will refuse to mount.
> > +
> > +Successfully decrypted disks are named as (cryptoX) and have linear
> > numeration
> > +with other decrypted by cryptodisk devices. If the encrypted disk hosts
> > some higher
> > +level abstraction (like LVM2 or MDRAID) it will be created under a
> > separate device
> > +namespace in addition to the cryptodisk namespace.
>
>
> This is standard cryptomount behavior. I think it makes more sense to
> have some version of this paragraph in cryptomount and make a note here
> that it behaves like cryptomount.
Do you mean to add this information in cryptomount doc as a part of this patch?
> > +#include <grub/cryptodisk.h>
> > +#include <grub/dl.h>
> > +#include <grub/err.h>
> > +#include <grub/extcmd.h>
> > +#include <grub/partition.h>
> > +#include <grub/file.h>
> > +
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +char PLAINMOUNT_DEFAULT_UUID[] = "109fea84-a6b7-34a8-4bd1-1c506305a400";
> > +#define BITS_PER_BYTE 8
> > +#define PLAINMOUNT_DEFAULT_SECTOR_SIZE 512
>
>
> Minor nit, I'd rather have these defines before the definition of
> PLAINMOUNT_DEFAULT_UUID.
The comment later says about making it a #define instead of char[]. I made it
char[] because it was more convenient for previous version of the code which
is not true in current version. Yes, I can change that.
>
> > +{
> > + grub_size_t pos = 0;
> > +
> > + /* Size of user_uuid is checked in main func /
> > + if (user_uuid)
> > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> > + else
> > + {
> > + /
> > + * Set default UUID. Last digits start from 1 and are incremented for
> > + * each new plainmount device.
> > + * snprintf() sets uuid to ' ...x' where x starts from 1.
> > + */
> > + grub_snprintf (dev->uuid, sizeof (dev->uuid)-1, "%36lx", dev->id+1);
> > + while (dev->uuid[pos++] == ' ');
> > + grub_memcpy (dev->uuid, PLAINMOUNT_DEFAULT_UUID, pos-1);
>
>
> I'm wondering if there's a good reason to have PLAINMOUNT_DEFAULT_UUID
> not be a #define, which I think makes more sense and would be expected
> if just reading this part of the code.
OK.
> > + if (dev->total_sectors == 0)
> > + return grub_error (GRUB_ERR_BAD_DEVICE, N_("cannot set specified sector
> > size on disk %s"),
> > + disk->name);
> > +
> > + /* Configure device offset */
> > + dev->offset_sectors = grub_divmod64 (offset, sector_size, NULL);
>
>
> Something I didn't catch before. Since there is only offset_sectors,
> there can be no non-sector aligned offsets. Thus it really doesn't make
> sense to have a '-o' option, blocklists will do everything that is
> needed, and more actually. If you have a plainmount volume with sector
> size 4096 at offset 2048 in (hd0), you can not access it using "-o
> 2048". That will cause grub_divmod64() above to return 0 (2048/4096).
> However creating a loopback device using "(hd0)+4" will work. This is
> partly a limitation of the current cryptodisk code. Perhaps there
> should be a dev->offset_partial that is the number of bytes to add to
>
> the offset_sectors to get the byte offset. That would be a different
> patch series though. And I'm not convinced that its very useful (if you
> have a convincing use-case I'm open to it).
I also think that -o option is useless in current conditions.
> > +
> > + if (len > size)
> > + len = size;
> > +
> > + grub_crypto_hash (dev->hash, dh, p, grub_strlen (p));
> > + }
> > + grub_free (p);
> > + return plainmount_setkey (dev, derived_hash, key_size);
> > +}
> > +
> > +
> > +/* Read key material from keyfile */
> > +static grub_err_t
> > +plainmount_configure_keyfile (grub_cryptodisk_t dev, char *keyfile,
> > grub_uint8_t *key_data,
>
>
> I don't think key_data needs to be passed here. Its only used to put
> data read from the key file, but never used outside of this function.
> My guess is that you're wanting to avoid allocating a buffer of
> GRUB_CRYPTODISK_MAX_PASSPHRASE twice. But its strange to have a
> function that takes a buffer that it unconditionally overwrites and
> where that buffer is not used after the function.
>
> Thinking about it more, plainmount_configure_password and
> plainmount_configure_keyfile should probably be refactored. I'm
> thinking that neither function should call plainmount_setkey(), but
> should have key_data as an out parameter and have plainmount_setkey()
> called from grub_cmd_plainmount(). I'm also open to something other
> than my above suggestion too.
I am OK with moving a call to setkey() outside of these two functions.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
