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

[Daniel Kiper]

On Wed, Nov 30, 2022 at 11:58:33PM -0600, Glenn Washburn wrote:
> On Tue, 29 Nov 2022 18:13:11 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > First of all, sorry for late reply. I hope I will be able to review
> > next version of this patch much faster.
> >
> > On Sat, Oct 29, 2022 at 05:40:42PM +0000, Maxim Fomin wrote:
> > > From 2b1d2deb3f2416cbc3e7d25cbc4141a3078eaf68 Mon Sep 17 00:00:00
> > > 2001 From: Maxim Fomin <maxim@fomin.one>
> > > Date: Sat, 29 Oct 2022 18:18:58 +0100
> > > Subject: [PATCH v8 1/1] plainmount: Support plain encryption mode
> > >
> > > This patch adds support for plain encryption mode (plain dm-crypt)
> > > via new module/command named 'plainmount'.
> > >
> > > Signed-off-by: Maxim Fomin <maxim@fomin.one>
> >
> > Please put "---" behind SOB and before "Difference with..." next
> > time...
>
> [...]
>
> > > +
> > > +/* 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().
> >
> > Is it safe to assume here that dev->uuid has been zeroed earlier?
>
> Yes, its zalloc'd in grub_cmd_plainmount below.

OK, cool!

[...]

> > > +  /* 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.
>
> Afaik, we don't really care what the UUID string is, just that its
> unique. It doesn't have to be a real UUID either. A minimum length
> check of 1 might be good, otherwise '--uuid ""' will create an empty
> uuid, but its not a big deal either. We might think about a character
> validation check so that characters like ')' can't be in the string.
> None of this will cause real problems as far as I can tell, but some
> features may not work right. For example, having ')' in the uuid will
> make it so that you can't access the crypto device via the
> '(cruuid/<uuid>)' syntax. I also don't mind not doing the validation
> and letting the user shoot themselves in the foot if they so choose.

I think we should check UUID size and probably it should not be smaller
than 1 and larger than "sizeof(PLAINMOUNT_DEFAULT_UUID) - 1". I do not
think we should check validity/content of the UUID.

Daniel