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

[Glenn Washburn]

Oct 23, 2020 1:00:59 PM Patrick Steinhardt <ps@pks.im>:

> 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])
>> +                - 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]));
>> +     iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_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_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 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
>>
>> +#define GRUB_TYPE_BIT(type) (sizeof(type) * GRUB_CHAR_BIT)
>
> Hum. I'd rather name this `GRUB_TYPE_BITS`, as the current name implies
> that we only get a single bit. Other than that the change looks good to
> me and I agree that it helps readability.
>
> Patrick

Yes, I would've liked to also, but I decided to stick with the naming 
convention as can be seen in the definition of GRUB_CHAR_BIT above it. Should 
there be a separate patch renaming GRUB_CHAR_BIT also?

>
>> +#define GRUB_TYPE_MAX(type) ((2 * ((1ULL << (GRUB_TYPE_BIT(type) - 1)) - 
>> 1)) + 1)
>> +
>> #endif /* ! GRUB_TYPES_HEADER */
>> --
>> 2.27.0
>>