1. Patchew
  2. QEMU
  3. View series

[PATCH] crypto: Always initialize splitkeylen

Akihiko Odaki posted 1 patch 11 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230522114737.32686-1-akihiko.odaki@daynix.com
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>
crypto/block-luks.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] crypto: Always initialize splitkeylen
Posted by Akihiko Odaki 11 months, 2 weeks ago 
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.

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
-- 
2.40.1
Re: [PATCH] crypto: Always initialize splitkeylen
Posted by Daniel P. Berrangé 11 months, 2 weeks ago 
On Mon, May 22, 2023 at 08:47:37PM +0900, 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.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  crypto/block-luks.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

and queued.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] crypto: Always initialize splitkeylen
Posted by Philippe Mathieu-Daudé 11 months, 2 weeks ago 
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
Re: [PATCH] crypto: Always initialize splitkeylen
Posted by Akihiko Odaki 11 months, 2 weeks ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.