Re: [PATCH v6 04/12] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt

[Glenn Washburn]

On Wed, 2 Dec 2020 18:37:42 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Nov 27, 2020 at 03:03:36AM -0600, Glenn Washburn wrote:
> > This should improve readability of code by providing clues as to
> > what the value represents. The new macro GRUB_TYPE_BITS(type)
> > returns the number of bits allocated for type. Also add
> > GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> > unsigned number with size of type.
> 
> Two separate patches please...

Ok.

> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 13 +++++++------
> >  include/grub/types.h        |  5 +++++
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 473c93976..31b73c535 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -284,22 +284,23 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, iv[1] = grub_cpu_to_le32 (sector >> 32);
> >       /* FALLTHROUGH */
> >     case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> > -     iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> > +     iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); break;
> >     case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> > -     iv[1] = grub_cpu_to_le32 (sector >> (32 -
> > dev->log_sector_size));
> > +     iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS
> > (iv[1])
> > +                                          -
> > dev->log_sector_size)); iv[0] = grub_cpu_to_le32 ((sector <<
> > dev->log_sector_size)
> > -                               & 0xFFFFFFFF);
> > +                               & GRUB_TYPE_U_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_BITS
> > (iv[0]));
> > +       iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX
> > (iv[0])); }
> >       break;
> >     case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> > -     iv[0] = grub_cpu_to_le32 (sector & 0xFFFFFFFF);
> > +     iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); 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 f22055f98..29d807f71 100644
> > --- a/include/grub/types.h
> > +++ b/include/grub/types.h
> > @@ -72,6 +72,8 @@
> >  # endif
> >  #endif
> >
> > +#define GRUB_TYPE_BITS(type) (sizeof(type) * GRUB_CHAR_BIT)
> 
> This macro should go below "#ifndef __CHAR_BIT__ ...".

Ack.

> >  #ifndef __CHAR_BIT__
> >  # error __CHAR_BIT__ is not defined
> >  #elif __CHAR_BIT__ != 8
> > @@ -159,6 +161,9 @@ typedef grub_int32_t    grub_ssize_t;
> >  #endif
> >  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
> >
> > +#define GRUB_TYPE_U_MAX(type) ((typeof (1ULL))((typeof
> > (type))(~0)))
> 
> (typeof (1ULL)) == (unsigned long long)?

I'll change it.

> > +#define GRUB_TYPE_U_MIN(type) 0ULL
> 
> I am not sure why you cast everything to ULL/unsigned long long.
> Should not the final cast be to (typeof (type))?

I think that to as great as an extent as possible macros should behave
like functions.  So in this case, its "return type" should not change
depending on the input arguments.  This can be confusing and the source
of errors.  Sure, you can say that the user of the macro can just do
the cast herself, but why add potentially unnecessary steps.

Also, I wanted to unsure that the result was an unsigned type,
regardless of signedness of input type. And using the largest unsigned
type seemed the best because I don't know how to take an arbitrary
integer type can only convert its signedness.

But at this point, I really don't care.  Tell me what you want it to be
and I'll make it so.

Glenn