Re: [PATCH 05/17] luks: Add support for LUKS2 in (proc)/luks_script

[Patrick Steinhardt]

On Wed, Jul 29, 2020 at 04:50:10PM -0500, development@efficientek.com wrote:
> From: Glenn Washburn <development@efficientek.com>
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c21be7d52..f6b6302e1 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1210,12 +1210,14 @@ luks_script_get (grub_size_t *sz)
>    *sz = 0;
>  
>    for (i = cryptodisk_list; i != NULL; i = i->next)
> -    if (grub_strcmp (i->modname, "luks") == 0)
> +    if (grub_strncmp (i->modname, "luks", 4) == 0)

While it works, I think it's a bit too magical. I'd personally prefer to
be a bit more verbose:

    if (grub_strcmp (i->modname, "luks") == 0 ||
        grub_strcmp (i->modname, "luks2") == 0)
      {
        ...
      }

>        {
> -     size += sizeof ("luks_mount ");
> +     size += grub_strlen (i->modname);
> +     size += sizeof ("_mount");
>       size += grub_strlen (i->uuid);
>       size += grub_strlen (i->cipher->cipher->name);
> -     size += 54;
> +     /* mode + mode_iv + spaces + offset + sector size + ??? + '\n' + NULL */
> +     size += 5 + 8 + 5 + 20 + 4 + 16 + 1 + 1;

Is it expected that the `size` is now bigger than before? This adds up
to `60` now. It's fine as it is more verbose than it previously has
been, but a comment in the commit message explaining that the different
size is intentional would've helped.

>       if (i->essiv_hash)
>         size += grub_strlen (i->essiv_hash->name);
>       size += i->keysize * 2;
> @@ -1228,16 +1230,16 @@ luks_script_get (grub_size_t *sz)
>    ptr = ret;
>  
>    for (i = cryptodisk_list; i != NULL; i = i->next)
> -    if (grub_strcmp (i->modname, "luks") == 0)
> +    if (grub_strncmp (i->modname, "luks", 4) == 0)

Same remark as above.

>        {
>       unsigned j;
>       const char *iptr;
> -     ptr = grub_stpcpy (ptr, "luks_mount ");
> +     ptr = grub_stpcpy (ptr, i->modname);
> +     ptr = grub_stpcpy (ptr, "_mount ");
>       ptr = grub_stpcpy (ptr, i->uuid);
>       *ptr++ = ' ';
> -     grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
> -     while (*ptr)
> -       ptr++;
> +     ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset);
> +     ptr += grub_snprintf (ptr, 6, "%d ", 1 << i->log_sector_size);
>       for (iptr = i->cipher->cipher->name; *iptr; iptr++)
>         *ptr++ = grub_tolower (*iptr);
>       switch (i->mode)
> -- 
> 2.25.1
>

signature.asc
Description: PGP signature