* [Qemu-devel] [PATCH] cipher: fix leak on initialization error
@ 2016-11-09 10:18 Marc-André Lureau
2016-11-09 10:22 ` Daniel P. Berrange
0 siblings, 1 reply; 3+ messages in thread
From: Marc-André Lureau @ 2016-11-09 10:18 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, Marc-André Lureau
If ctx->blocksize != XTS_BLOCK_SIZE, ctx will be leaked.
Assign ctx earler, and call qcrypto_cipher_free() on error.
Spotted thanks to ASAN.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
crypto/cipher-nettle.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
index cd094cd..593962c 100644
--- a/crypto/cipher-nettle.c
+++ b/crypto/cipher-nettle.c
@@ -376,6 +376,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
goto error;
}
+ cipher->opaque = ctx;
if (mode == QCRYPTO_CIPHER_MODE_XTS &&
ctx->blocksize != XTS_BLOCK_SIZE) {
error_setg(errp, "Cipher block size %zu must equal XTS block size %d",
@@ -384,13 +385,11 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
}
ctx->iv = g_new0(uint8_t, ctx->blocksize);
- cipher->opaque = ctx;
return cipher;
error:
- g_free(cipher);
- g_free(ctx);
+ qcrypto_cipher_free(cipher);
return NULL;
}
@@ -404,10 +403,12 @@ void qcrypto_cipher_free(QCryptoCipher *cipher)
}
ctx = cipher->opaque;
- g_free(ctx->iv);
- g_free(ctx->ctx);
- g_free(ctx->ctx_tweak);
- g_free(ctx);
+ if (ctx) {
+ g_free(ctx->iv);
+ g_free(ctx->ctx);
+ g_free(ctx->ctx_tweak);
+ g_free(ctx);
+ }
g_free(cipher);
}
--
2.10.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] cipher: fix leak on initialization error
2016-11-09 10:18 [Qemu-devel] [PATCH] cipher: fix leak on initialization error Marc-André Lureau
@ 2016-11-09 10:22 ` Daniel P. Berrange
2016-11-09 10:26 ` Marc-André Lureau
0 siblings, 1 reply; 3+ messages in thread
From: Daniel P. Berrange @ 2016-11-09 10:22 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel
On Wed, Nov 09, 2016 at 02:18:15PM +0400, Marc-André Lureau wrote:
> If ctx->blocksize != XTS_BLOCK_SIZE, ctx will be leaked.
> Assign ctx earler, and call qcrypto_cipher_free() on error.
>
> Spotted thanks to ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> crypto/cipher-nettle.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
> index cd094cd..593962c 100644
> --- a/crypto/cipher-nettle.c
> +++ b/crypto/cipher-nettle.c
> @@ -376,6 +376,7 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> goto error;
> }
'ctx' is non-NULL at this point and there's a 'goto error' just
above here....
>
> + cipher->opaque = ctx;
> if (mode == QCRYPTO_CIPHER_MODE_XTS &&
> ctx->blocksize != XTS_BLOCK_SIZE) {
> error_setg(errp, "Cipher block size %zu must equal XTS block size %d",
> @@ -384,13 +385,11 @@ QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> }
>
> ctx->iv = g_new0(uint8_t, ctx->blocksize);
> - cipher->opaque = ctx;
>
> return cipher;
>
> error:
> - g_free(cipher);
> - g_free(ctx);
> + qcrypto_cipher_free(cipher);
> return NULL;
> }
...so you're leaking 'ctx' now, since it hasn't been assigned
to cipher->ctx.
You need to move 'cipher->opque = ctx' to the place where we
initially allocate 'ctx', before any gotos at which point....
>
> @@ -404,10 +403,12 @@ void qcrypto_cipher_free(QCryptoCipher *cipher)
> }
>
> ctx = cipher->opaque;
> - g_free(ctx->iv);
> - g_free(ctx->ctx);
> - g_free(ctx->ctx_tweak);
> - g_free(ctx);
> + if (ctx) {
> + g_free(ctx->iv);
> + g_free(ctx->ctx);
> + g_free(ctx->ctx_tweak);
> + g_free(ctx);
> + }
> g_free(cipher);
> }
...this change is not needed
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] cipher: fix leak on initialization error
2016-11-09 10:22 ` Daniel P. Berrange
@ 2016-11-09 10:26 ` Marc-André Lureau
0 siblings, 0 replies; 3+ messages in thread
From: Marc-André Lureau @ 2016-11-09 10:26 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Marc-André Lureau, qemu-devel
Hi
----- Original Message -----
> On Wed, Nov 09, 2016 at 02:18:15PM +0400, Marc-André Lureau wrote:
> > If ctx->blocksize != XTS_BLOCK_SIZE, ctx will be leaked.
> > Assign ctx earler, and call qcrypto_cipher_free() on error.
> >
> > Spotted thanks to ASAN.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > crypto/cipher-nettle.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/crypto/cipher-nettle.c b/crypto/cipher-nettle.c
> > index cd094cd..593962c 100644
> > --- a/crypto/cipher-nettle.c
> > +++ b/crypto/cipher-nettle.c
> > @@ -376,6 +376,7 @@ QCryptoCipher
> > *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> > goto error;
> > }
>
> 'ctx' is non-NULL at this point and there's a 'goto error' just
> above here....
>
Right, fixing it sending v2
> >
> > + cipher->opaque = ctx;
> > if (mode == QCRYPTO_CIPHER_MODE_XTS &&
> > ctx->blocksize != XTS_BLOCK_SIZE) {
> > error_setg(errp, "Cipher block size %zu must equal XTS block size
> > %d",
> > @@ -384,13 +385,11 @@ QCryptoCipher
> > *qcrypto_cipher_new(QCryptoCipherAlgorithm alg,
> > }
> >
> > ctx->iv = g_new0(uint8_t, ctx->blocksize);
> > - cipher->opaque = ctx;
> >
> > return cipher;
> >
> > error:
> > - g_free(cipher);
> > - g_free(ctx);
> > + qcrypto_cipher_free(cipher);
> > return NULL;
> > }
>
>
> ...so you're leaking 'ctx' now, since it hasn't been assigned
> to cipher->ctx.
>
> You need to move 'cipher->opque = ctx' to the place where we
> initially allocate 'ctx', before any gotos at which point....
>
> >
> > @@ -404,10 +403,12 @@ void qcrypto_cipher_free(QCryptoCipher *cipher)
> > }
> >
> > ctx = cipher->opaque;
> > - g_free(ctx->iv);
> > - g_free(ctx->ctx);
> > - g_free(ctx->ctx_tweak);
> > - g_free(ctx);
> > + if (ctx) {
> > + g_free(ctx->iv);
> > + g_free(ctx->ctx);
> > + g_free(ctx->ctx_tweak);
> > + g_free(ctx);
> > + }
> > g_free(cipher);
> > }
>
> ...this change is not needed
>
> Regards,
> Daniel
> --
> |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org -o- http://virt-manager.org :|
> |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-09 10:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-09 10:18 [Qemu-devel] [PATCH] cipher: fix leak on initialization error Marc-André Lureau
2016-11-09 10:22 ` Daniel P. Berrange
2016-11-09 10:26 ` Marc-André Lureau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).