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

[Maxim Fomin]

------- Original Message -------
On Tuesday, November 29th, 2022 at 20:13, Daniel Kiper <dkiper@net-space.pl> 
wrote:

> Hmmm... Why do you say "Writing data" here if the GRUB does not support
> filesystems writes except grubenv file? I think it should be clarified
> what you mean or dropped if it is wrong in this context. Otherwise it is
> confusing.

I do not know 100% of GRUB to be sure that none of modules can write data
(can be used to write data) to unformatted disk partitions. Note, the risk
here is not to write to some proper fs, but to write data at arbitrary disk
or partition location which happens to be incorrectly decrypted fs containing
some data (wrongly decrypted plain mode device looks like random data). If
there is nothing in GRUB which can write data at arbitrary disk locations,
then the sentence can be indeed removed. Although writing data at arbitrary
locations is a strange thing to do, doing so on unformatted disk/partition is
safe, while doing so on incorrectly decrypted in plain mode device will damage
encrypted data. If using plain mode considered expert level, then the warning
may be not needed because it is assumed that a user knows what he is doing.


> > +/ Configure cryptodisk uuid */
> > +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char 
> > user_uuid)
> > +{
> > + grub_size_t pos = 0;
> > +
> > + / Size of user_uuid is checked in main func */
> > + if (user_uuid != NULL)
> > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
> 
> 
> I think grub_strcpy() instead of grub_memcpy() would be more natural in
> string contexts. And then you do not need grub_strlen().

OK.
 
> Is it safe to assume here that dev->uuid has been zeroed earlier?

It seems Glenn has already answered here.

> > + / Check uuid length */
> > + if (state[OPTION_UUID].set && grub_strlen (state[OPTION_UUID].arg) >
> > + sizeof (PLAINMOUNT_DEFAULT_UUID))
> > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > + N_("specified UUID exceeds maximum size"));
> 
> 
> What will happen if somebody passes shorter, probably non-valid, UUID?
> I think we should check UUID is not shorter than something too.

It seems there is agreement to check that UUID size is longer than 1 and less
than PLAINMOUNT_UUID - 1.

Best regards,
Maxim Fomin