Re: [PATCH v3 07/10] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt.

[Daniel Kiper]

On Mon, Oct 19, 2020 at 06:09:55PM -0500, Glenn Washburn wrote:
> This should improve readability of code by providing clues as to what the
> value represents.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 12 +++++++-----
>  include/grub/types.h        |  3 +++
>  2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 623f0f396..1a91c2d55 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -297,19 +297,21 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
>         }
>         break;
>       case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> -       iv[1] = grub_cpu_to_le32 (sector >> (32 - log_sector_size));
> +       /* The IV is the 64 bit byte offset of the sector. */
> +       iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BIT(iv[0])

Missing space between macro name and "(".

> +                                            - log_sector_size));
>         iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
> -                                 & 0xFFFFFFFF);
> +                                 & GRUB_TYPE_MAX(iv[0]));
>         break;
>       case GRUB_CRYPTODISK_MODE_IV_BENBI:
>         {
>           grub_uint64_t num = (sector << dev->benbi_log) + 1;
> -         iv[sz - 2] = grub_cpu_to_be32 (num >> 32);
> -         iv[sz - 1] = grub_cpu_to_be32 (num & 0xFFFFFFFF);
> +         iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BIT(iv[0]));

Ditto.

> +         iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_MAX(iv[0]));

Ditto.

>         }
>         break;
>       case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> -       iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> +       iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_MAX(iv[0]));

Ditto.

>         err = grub_crypto_ecb_encrypt (dev->essiv_cipher, iv, iv,
>                                        dev->cipher->cipher->blocksize);
>         if (err)
> diff --git a/include/grub/types.h b/include/grub/types.h
> index 035a4b528..8b4267ebd 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -319,4 +319,7 @@ static inline void grub_set_unaligned64 (void *ptr, 
> grub_uint64_t val)
>
>  #define GRUB_CHAR_BIT 8

I think we should not change the name of this constant. It seems to me that
it was named in similar way to C/gcc/clang CHAR_BIT and/or __CHAR_BIT__.
However, I think we should change its definition. It should be

  #define GRUB_CHAR_BIT __CHAR_BIT__

Additionally, the build should stop if __CHAR_BIT__ is not defined nor
GRUB_CHAR_BIT is not equal 8.

Last but not least, the GRUB_CHAR_BIT definition should be right behind includes
in the include/grub/types.h.

> +#define GRUB_TYPE_BIT(type) (sizeof(type) * GRUB_CHAR_BIT)

I agree with Patrick, it should be named GRUB_TYPE_BITS.

> +#define GRUB_TYPE_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BIT(type) - 1)) - 1)) 
> + 1)

This only works for unsigned types. So, I think we should have three
(four?) macros here: (GRUB_TYPE_U_MIN? (which is obviously 0)),
GRUB_TYPE_U_MAX, GRUB_TYPE_S_MIN and GRUB_TYPE_S_MAX.

Daniel