Re: [PATCH] crypto: Always initialize splitkeylen

[Philippe Mathieu-Daudé]

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?

-- >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