From: Jesper Juhl Subject: RE: [PATCH] rfc4106, Intel, AES-NI: Don't leak memory in rfc4106_set_hash_subkey(). Date: Sun, 6 Feb 2011 20:43:22 +0100 (CET) Message-ID: References: <24483BA4C9F69C43A7379639D35543D7C5398DB6@irsmsx502.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: "linux-crypto@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "H. Peter Anvin" , Ingo Molnar , Thomas Gleixner , "David S. Miller" , "Huang, Ying" , "Hoban, Adrian" , "Paoloni, Gabriele" , "O Mahony, Aidan" To: "Struk, Tadeusz" , Herbert Xu Return-path: Received: from swampdragon.chaosbits.net ([90.184.90.115]:21396 "EHLO swampdragon.chaosbits.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753645Ab1BFToj (ORCPT ); Sun, 6 Feb 2011 14:44:39 -0500 In-Reply-To: <24483BA4C9F69C43A7379639D35543D7C5398DB6@irsmsx502.ger.corp.intel.com> Sender: linux-crypto-owner@vger.kernel.org List-ID: On Wed, 19 Jan 2011, Struk, Tadeusz wrote: > Hi Jesper, Hi Tadeusz, > We have tested your changes and all is fine. > Thank you for identifying this issue. You are welcome. But I believe I made a mistake in that patch. I looked at it again and I'm convinced that the 'out' label should not exist. The jump to it will still leak 'ctr_tfm' - don't you agree? I believe the patch should look like this instead: Signed-off-by: Jesper Juhl --- aesni-intel_glue.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c index e1e60c7..4a8c015 100644 --- a/arch/x86/crypto/aesni-intel_glue.c +++ b/arch/x86/crypto/aesni-intel_glue.c @@ -873,21 +873,19 @@ rfc4106_set_hash_subkey(u8 *hash_subkey, const u8 *key, unsigned int key_len) crypto_ablkcipher_clear_flags(ctr_tfm, ~0); ret = crypto_ablkcipher_setkey(ctr_tfm, key, key_len); - if (ret) { - crypto_free_ablkcipher(ctr_tfm); - return ret; - } + if (ret) + goto out_free_ablkcipher; req = ablkcipher_request_alloc(ctr_tfm, GFP_KERNEL); if (!req) { - crypto_free_ablkcipher(ctr_tfm); - return -EINVAL; + ret = -EINVAL; + goto out_free_ablkcipher; } req_data = kmalloc(sizeof(*req_data), GFP_KERNEL); if (!req_data) { - crypto_free_ablkcipher(ctr_tfm); - return -ENOMEM; + ret = -ENOMEM; + goto out_free_request; } memset(req_data->iv, 0, sizeof(req_data->iv)); @@ -913,8 +911,10 @@ rfc4106_set_hash_subkey(u8 *hash_subkey, const u8 *key, unsigned int key_len) if (!ret) ret = req_data->result.err; } +out_free_request: ablkcipher_request_free(req); kfree(req_data); +out_free_ablkcipher: crypto_free_ablkcipher(ctr_tfm); return ret; } Herbert: If Tadeusz agrees, could you please replace the patch you merged with the one above? -- Jesper Juhl http://www.chaosbits.net/ Plain text mails only, please. Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html