Re: [PATCH v4 11/15] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt.

[Glenn Washburn]

On Sun, 15 Nov 2020 10:59:33 +0100
Patrick Steinhardt <ps@pks.im> wrote:

> On Fri, Nov 06, 2020 at 10:44:31PM -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.
> > 
> > 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 495c86bb1..b57acc3b1 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)
> > +
> >  #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) ((2 * ((1ULL << (GRUB_TYPE_BITS
> > (type) - 1)) - 1)) + 1)
> 
> This macro is quite complicated. Considering this is specific to
> unsigned types now, couldn't we just use the following?
> 
> #define GRUB_TYPE_U_MAX(type) (~((type)0))
> 
> Patrick

The way its currently defined works for arbitrary bit lengths.  I
suppose its safe to assume that it would only be needed for bit lengths
of actual types.

And your suggestion won't quite work because the bit-complement
operator promotes its argument to an int (or an unsigned int if it
won't fit in an int). So if type is unsigned short, then ~((unsigned
short)0) -> ~((int)0x0) -> 0xffffffff.

However, I think you've hit on a reasonable idea.  This looks like
it'll do what you were intending:

#define GRUB_TYPE_U_MAX(type) ((typeof (1ULL))((typeof (type))(~0)))

I'll update with this.

Glenn

> 
> > +#define GRUB_TYPE_U_MIN(type) 0ULL
> > +
> >  typedef grub_uint64_t grub_properly_aligned_t;
> >  
> >  #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size)
> > grub_properly_aligned_t name[((size) + sizeof
> > (grub_properly_aligned_t) - 1) / sizeof (grub_properly_aligned_t)]
> > -- 2.27.0
> >