Re: [PATCH] crypto: Always initialize splitkeylen

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/05/22 21:35, Philippe Mathieu-Daudé wrote:
> On 22/5/23 13:47, Akihiko Odaki wrote:
>> When _FORTIFY_SOURCE=2, glibc version is 2.35, and GCC version is
>> 12.1.0, the compiler complains as follows:
>>
>> In file included from /usr/include/string.h:535,
>>                   from /home/alarm/q/var/qemu/include/qemu/osdep.h:99,
>>                   from ../crypto/block-luks.c:21:
>> In function 'memset',
>>      inlined from 'qcrypto_block_luks_store_key' at 
>> ../crypto/block-luks.c:843:9:
>> /usr/include/bits/string_fortified.h:59:10: error: 'splitkeylen' may 
>> be used uninitialized [-Werror=maybe-uninitialized]
>>     59 |   return __builtin___memset_chk (__dest, __ch, __len,
>>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>     60 |                                  __glibc_objsize0 (__dest));
>>        |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../crypto/block-luks.c: In function 'qcrypto_block_luks_store_key':
>> ../crypto/block-luks.c:699:12: note: 'splitkeylen' was declared here
>>    699 |     size_t splitkeylen;
>>        |            ^~~~~~~~~~~
>>
>> It seems the compiler cannot see that splitkeylen will not be used
>> when splitkey is NULL. Suppress the warning by initializing splitkeylen
>> even when splitkey stays NULL.
> 
> What about using splitkeylen instead?

splitkey won't be allocated soon after initializing splitkeylen so using 
splitkeylen can result in NULL dereference.

Besides I think it's better not to initialize splitkeylen with 0 because 
the compiler will not warn even if you mistakenly used the value; if you 
leave the variable uninitialized, the compiler can warn of such a 
mistake. The same logic also applies to the NULL initialization of 
splitkey, but NULL is somewhat safer as dereferencing it results in 
segfault and automatically behaves like an assertion. It seems splitkey 
needs to be initialized anyway because it has g_autofree as you pointed 
out for another patch.

> 
> -- >8 --
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index 5688783ab1..dfba98fdc1 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -696,7 +696,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
>       QCryptoBlockLUKS *luks = block->opaque;
>       QCryptoBlockLUKSKeySlot *slot;
>       g_autofree uint8_t *splitkey = NULL;
> -    size_t splitkeylen;
> +    size_t splitkeylen = 0;
>       g_autofree uint8_t *slotkey = NULL;
>       g_autoptr(QCryptoCipher) cipher = NULL;
>       g_autoptr(QCryptoIVGen) ivgen = NULL;
> @@ -839,7 +839,7 @@ cleanup:
>       if (slotkey) {
>           memset(slotkey, 0, luks->header.master_key_len);
>       }
> -    if (splitkey) {
> +    if (splitkeylen) {
>           memset(splitkey, 0, splitkeylen);
>       }
>       return ret;
> ---
> 
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   crypto/block-luks.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
>> index 5688783ab1..2f59c3a625 100644
>> --- a/crypto/block-luks.c
>> +++ b/crypto/block-luks.c
>> @@ -706,14 +706,14 @@ qcrypto_block_luks_store_key(QCryptoBlock *block,
>>       assert(slot_idx < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
>>       slot = &luks->header.key_slots[slot_idx];
>> +    splitkeylen = luks->header.master_key_len * slot->stripes;
>> +
>>       if (qcrypto_random_bytes(slot->salt,
>>                                QCRYPTO_BLOCK_LUKS_SALT_LEN,
>>                                errp) < 0) {
>>           goto cleanup;
>>       }
>> -    splitkeylen = luks->header.master_key_len * slot->stripes;
>> -
>>       /*
>>        * Determine how many iterations are required to
>>        * hash the user password while consuming 1 second of compute
>