Re: [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script

[Daniel Kiper]

On Sun, Feb 05, 2023 at 01:05:31AM -0600, Glenn Washburn wrote:
> The sector size in bytes is added to each line and it is allowed to be 5
> decimal digits long, which covers the most common cases of 512 and 4096
> byte sectors with space for an additional digit as future-proofing. The
> size allocation is updated to reflect this additional field, allow up to
> 5 characters and 1 space added.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 34b67a705f..f2c8c8d3fe 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1478,12 +1478,22 @@ 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_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;
> +     /*
> +      * Add space in the line for (in order) spaces, cipher mode, cipher IV
> +      * mode, sector offset, sector size and the trailing newline. This is
> +      * an upper bound on the size of this data. There are 16 extra bytes
> +      * in an earlier version of this code that are unaccounted for. It is
> +      * left in the calculations in case it is needed. At worst, its short-
> +      * lived wasted space.
> +      */
> +     size += 5 + 5 + 8 + 20 + 5 + 1 + 16;
>       if (i->essiv_hash)
>         size += grub_strlen (i->essiv_hash->name);
>       size += i->keysize * 2;
> @@ -1496,16 +1506,18 @@ 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_strcmp (i->modname, "luks") == 0 ||
> +     grub_strcmp (i->modname, "luks2") == 0)
>        {
>       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_sectors);
> -     while (*ptr)
> -       ptr++;
> +     ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ",
> +                           i->offset_sectors);

You, OK not you :-), blindly assume offset_sectors is 64-bit long. Today
it is but in the future it may not. May I ask you to define PRIuGRUB_DISK_ADDR,
etc. properly and fix the problem here and in the other places if needed?

> +     ptr += grub_snprintf (ptr, 7, "%d ", 1 << i->log_sector_size);

Ugh... Simple "grep -Ir log_sector_size include" revealed we use
different types in different places for log_sector_size. The most
worrying for me are include/grub/disk.h and include/grub/cryptodisk.h.
OK, probably it is not big problem here but I would prefer if we
are consistent all over the place. I think it would be good to do
"typedef grub_uint32_t grub_disk_secs_t" and use grub_disk_secs_t
consistently everywhere. Could you do that? Of course it can be
a separate patch set...

Daniel