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

[Daniel Kiper]

On Thu, Dec 03, 2020 at 02:29:11AM -0600, Glenn Washburn wrote:
> 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.

OK, make sense for me. However, please put short comment before macros
why you are casting final result to unsigned long long.

Daniel