[PATCH] crypto/block-luks: always set splitkeylen to 0

[PATCH] crypto/block-luks: always set splitkeylen to 0

[Patrick Venture]

This was caught by a sanitized build, that was perhaps oversensitive.

Signed-off-by: Patrick Venture <venture@google.com>
---
 crypto/block-luks.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index f62be6836b..8633fb7e9f 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -729,7 +729,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;
@@ -901,7 +901,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
     QCryptoBlockLUKS *luks = block->opaque;
     const QCryptoBlockLUKSKeySlot *slot;
     g_autofree uint8_t *splitkey = NULL;
-    size_t splitkeylen;
+    size_t splitkeylen = 0;
     g_autofree uint8_t *possiblekey = NULL;
     int rv;
     g_autoptr(QCryptoCipher) cipher = NULL;
@@ -1147,7 +1147,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
     QCryptoBlockLUKS *luks = block->opaque;
     QCryptoBlockLUKSKeySlot *slot;
     g_autofree uint8_t *garbagesplitkey = NULL;
-    size_t splitkeylen;
+    size_t splitkeylen = 0;
     size_t i;
     Error *local_err = NULL;
     int ret;
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH] crypto/block-luks: always set splitkeylen to 0

[Daniel P. Berrangé]

On Wed, Sep 07, 2022 at 09:21:25AM -0700, Patrick Venture wrote:
> This was caught by a sanitized build, that was perhaps oversensitive.
> 
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>  crypto/block-luks.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> index f62be6836b..8633fb7e9f 100644
> --- a/crypto/block-luks.c
> +++ b/crypto/block-luks.c
> @@ -729,7 +729,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;
> @@ -901,7 +901,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
>      QCryptoBlockLUKS *luks = block->opaque;
>      const QCryptoBlockLUKSKeySlot *slot;
>      g_autofree uint8_t *splitkey = NULL;
> -    size_t splitkeylen;
> +    size_t splitkeylen = 0;
>      g_autofree uint8_t *possiblekey = NULL;
>      int rv;
>      g_autoptr(QCryptoCipher) cipher = NULL;
> @@ -1147,7 +1147,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
>      QCryptoBlockLUKS *luks = block->opaque;
>      QCryptoBlockLUKSKeySlot *slot;
>      g_autofree uint8_t *garbagesplitkey = NULL;
> -    size_t splitkeylen;
> +    size_t splitkeylen = 0;
>      size_t i;
>      Error *local_err = NULL;
>      int ret;

In all three cases, splitkeylen is initialized a few lines later.

In qcrypto_block_luks_store_key there is a 'goto cleanup' before
the initialization. The 'cleanup' code can use 'splitkeylen', but
only if 'splitkey != NULL' & this isn't possible if splitkeylen is
uninitialized.

The other two methods have no code path where splitkeylen can be
used uninitialized.

The tool is reporting non-existant problems AFAICT

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/block-luks: always set splitkeylen to 0

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 2710 bytes --]

On Wed, Sep 7, 2022 at 9:34 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Sep 07, 2022 at 09:21:25AM -0700, Patrick Venture wrote:
> > This was caught by a sanitized build, that was perhaps oversensitive.
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >  crypto/block-luks.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index f62be6836b..8633fb7e9f 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -729,7 +729,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;
> > @@ -901,7 +901,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block,
> >      QCryptoBlockLUKS *luks = block->opaque;
> >      const QCryptoBlockLUKSKeySlot *slot;
> >      g_autofree uint8_t *splitkey = NULL;
> > -    size_t splitkeylen;
> > +    size_t splitkeylen = 0;
> >      g_autofree uint8_t *possiblekey = NULL;
> >      int rv;
> >      g_autoptr(QCryptoCipher) cipher = NULL;
> > @@ -1147,7 +1147,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block,
> >      QCryptoBlockLUKS *luks = block->opaque;
> >      QCryptoBlockLUKSKeySlot *slot;
> >      g_autofree uint8_t *garbagesplitkey = NULL;
> > -    size_t splitkeylen;
> > +    size_t splitkeylen = 0;
> >      size_t i;
> >      Error *local_err = NULL;
> >      int ret;
>
> In all three cases, splitkeylen is initialized a few lines later.
>
> In qcrypto_block_luks_store_key there is a 'goto cleanup' before
> the initialization. The 'cleanup' code can use 'splitkeylen', but
> only if 'splitkey != NULL' & this isn't possible if splitkeylen is
> uninitialized.
>
> The other two methods have no code path where splitkeylen can be
> used uninitialized.
>
> The tool is reporting non-existant problems AFAICT
>

I'm 100% certain it is. I just needed to make the changes to get a
sanitized build of Qemu to build.  Presumably if someone else tries
building qemu with `--enable-sanitizers` they'll have to make the same
non-sense adjustment.


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

[-- Attachment #2: Type: text/html, Size: 4082 bytes --]