[PATCH 0/3] Cleanup ressource leaks in test_aead_speed()

[PATCH 0/3] Cleanup ressource leaks in test_aead_speed()

[Christian Engelmayer]

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

This is a cleanup of Coverity ressource leak findings for the quick & dirty
crypto testing module crypto/tcrypt.c.

All 3 changesets address function test_aead_speed() that was introduced in
53f52d7a (crypto: tcrypt - Added speed tests for AEAD crypto alogrithms in
tcrypt test suite)

The series applies against v3.15-rc2 as well as branch master in tree
git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git and is
compile tested only.

Christian Engelmayer (3):
  crypto: Fix potential leak in test_aead_speed() if aad_size is too big
  crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails
  crypto: Fix leak of struct aead_request in test_aead_speed()

 crypto/tcrypt.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big

[Christian Engelmayer]

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

Fix a potential memory leak in the error handling of test_aead_speed(). In case
the size check on the associate data length parameter fails, the function goes
through the wrong exit label. Reported by Coverity - CID 1163870.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
 crypto/tcrypt.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 870be7b..1856d7f 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 	unsigned int *b_size;
 	unsigned int iv_len;
 
+	if (aad_size >= PAGE_SIZE) {
+		pr_err("associate data length (%u) too big\n", aad_size);
+		return;
+	}
+
 	if (enc == ENCRYPT)
 		e = "encryption";
 	else
@@ -323,14 +328,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 		b_size = aead_sizes;
 		do {
 			assoc = axbuf[0];
-
-			if (aad_size < PAGE_SIZE)
-				memset(assoc, 0xff, aad_size);
-			else {
-				pr_err("associate data length (%u) too big\n",
-					aad_size);
-				goto out_nosg;
-			}
+			memset(assoc, 0xff, aad_size);
 			sg_init_one(&asg[0], assoc, aad_size);
 
 			if ((*keysize + *b_size) > TVMEMSIZE * PAGE_SIZE) {
-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails

[Christian Engelmayer]

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

Fix a potential memory leak in the error handling of test_aead_speed(). In case
crypto_alloc_aead() fails, the function returns without going through the
centralized cleanup path. Reported by Coverity - CID 1163870.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
 crypto/tcrypt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1856d7f..1849155 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -313,7 +313,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 	if (IS_ERR(tfm)) {
 		pr_err("alg: aead: Failed to load transform for %s: %ld\n", algo,
 		       PTR_ERR(tfm));
-		return;
+		goto out_notfm;
 	}
 
 	req = aead_request_alloc(tfm, GFP_KERNEL);
@@ -391,6 +391,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 
 out:
 	crypto_free_aead(tfm);
+out_notfm:
 	kfree(sg);
 out_nosg:
 	testmgr_free_buf(xoutbuf);
-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/3] crypto: Fix leak of struct aead_request in test_aead_speed()

[Christian Engelmayer]

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

Fix leakage of memory for struct aead_request that is allocated via
aead_request_alloc() but not released via aead_request_free().
Reported by Coverity - CID 1163869.

Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
---
 crypto/tcrypt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
index 1849155..09c93ff2 100644
--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -320,7 +320,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 	if (!req) {
 		pr_err("alg: aead: Failed to allocate request for %s\n",
 		       algo);
-		goto out;
+		goto out_noreq;
 	}
 
 	i = 0;
@@ -390,6 +390,8 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
 	} while (*keysize);
 
 out:
+	aead_request_free(req);
+out_noreq:
 	crypto_free_aead(tfm);
 out_notfm:
 	kfree(sg);
-- 
1.9.1

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big

[Marek Vasut]

On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In
> case the size check on the associate data length parameter fails, the
> function goes through the wrong exit label. Reported by Coverity - CID
> 1163870.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>
> ---
>  crypto/tcrypt.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 870be7b..1856d7f 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc,
> unsigned int sec, unsigned int *b_size;
>  	unsigned int iv_len;
> 
> +	if (aad_size >= PAGE_SIZE) {

On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient here?

Cheers!

Re: [PATCH 2/3] crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails

[Marek Vasut]

On Monday, April 21, 2014 at 08:46:40 PM, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In
> case crypto_alloc_aead() fails, the function returns without going through
> the centralized cleanup path. Reported by Coverity - CID 1163870.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

Looks OK to me, thanks.

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 3/3] crypto: Fix leak of struct aead_request in test_aead_speed()

[Marek Vasut]

On Monday, April 21, 2014 at 08:47:05 PM, Christian Engelmayer wrote:
> Fix leakage of memory for struct aead_request that is allocated via
> aead_request_alloc() but not released via aead_request_free().
> Reported by Coverity - CID 1163869.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big

[Tim Chen]

On Mon, 2014-04-21 at 20:45 +0200, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In case
> the size check on the associate data length parameter fails, the function goes
> through the wrong exit label. Reported by Coverity - CID 1163870.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

Acked.

Tim

> ---
>  crypto/tcrypt.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 870be7b..1856d7f 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -282,6 +282,11 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  	unsigned int *b_size;
>  	unsigned int iv_len;
>  
> +	if (aad_size >= PAGE_SIZE) {
> +		pr_err("associate data length (%u) too big\n", aad_size);
> +		return;
> +	}
> +
>  	if (enc == ENCRYPT)
>  		e = "encryption";
>  	else
> @@ -323,14 +328,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  		b_size = aead_sizes;
>  		do {
>  			assoc = axbuf[0];
> -
> -			if (aad_size < PAGE_SIZE)
> -				memset(assoc, 0xff, aad_size);
> -			else {
> -				pr_err("associate data length (%u) too big\n",
> -					aad_size);
> -				goto out_nosg;
> -			}
> +			memset(assoc, 0xff, aad_size);
>  			sg_init_one(&asg[0], assoc, aad_size);
>  
>  			if ((*keysize + *b_size) > TVMEMSIZE * PAGE_SIZE) {

Re: [PATCH 2/3] crypto: Fix potential leak in test_aead_speed() if crypto_alloc_aead() fails

[Tim Chen]

On Mon, 2014-04-21 at 20:46 +0200, Christian Engelmayer wrote:
> Fix a potential memory leak in the error handling of test_aead_speed(). In case
> crypto_alloc_aead() fails, the function returns without going through the
> centralized cleanup path. Reported by Coverity - CID 1163870.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

Acked.

Tim

> ---
>  crypto/tcrypt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 1856d7f..1849155 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -313,7 +313,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  	if (IS_ERR(tfm)) {
>  		pr_err("alg: aead: Failed to load transform for %s: %ld\n", algo,
>  		       PTR_ERR(tfm));
> -		return;
> +		goto out_notfm;
>  	}
>  
>  	req = aead_request_alloc(tfm, GFP_KERNEL);
> @@ -391,6 +391,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  
>  out:
>  	crypto_free_aead(tfm);
> +out_notfm:
>  	kfree(sg);
>  out_nosg:
>  	testmgr_free_buf(xoutbuf);

Re: [PATCH 3/3] crypto: Fix leak of struct aead_request in test_aead_speed()

[Tim Chen]

On Mon, 2014-04-21 at 20:47 +0200, Christian Engelmayer wrote:
> Fix leakage of memory for struct aead_request that is allocated via
> aead_request_alloc() but not released via aead_request_free().
> Reported by Coverity - CID 1163869.
> 
> Signed-off-by: Christian Engelmayer <cengelma@gmx.at>

Acked.  Thanks for fixing the leak.

Tim

> ---
>  crypto/tcrypt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index 1849155..09c93ff2 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -320,7 +320,7 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  	if (!req) {
>  		pr_err("alg: aead: Failed to allocate request for %s\n",
>  		       algo);
> -		goto out;
> +		goto out_noreq;
>  	}
>  
>  	i = 0;
> @@ -390,6 +390,8 @@ static void test_aead_speed(const char *algo, int enc, unsigned int sec,
>  	} while (*keysize);
>  
>  out:
> +	aead_request_free(req);
> +out_noreq:
>  	crypto_free_aead(tfm);
>  out_notfm:
>  	kfree(sg);

Re: [PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big

[Christian Engelmayer]

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

On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut <marex@denx.de> wrote:
> On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> > +	if (aad_size >= PAGE_SIZE) {
> 
> On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient here?

From what I have seen how the buffers are allocated via __get_free_page() I
thought so too. However, as it previously read

	if (aad_size < PAGE_SIZE)
		memset(assoc, 0xff, aad_size);
	else {

my intention was simply to make the modification so that the bug is addressed
without introducing an additional change.

Regards,
Christian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] crypto: Fix potential leak in test_aead_speed() if aad_size is too big

[Marek Vasut]

On Wednesday, April 23, 2014 at 07:43:35 PM, Christian Engelmayer wrote:
> On Wed, 23 Apr 2014 01:33:05 +0200, Marek Vasut <marex@denx.de> wrote:
> > On Monday, April 21, 2014 at 08:45:59 PM, Christian Engelmayer wrote:
> > > +	if (aad_size >= PAGE_SIZE) {
> > 
> > On an unrelated note ... Won't if (aad_size > PAGE_SIZE) be sufficient
> > here?
> 
> From what I have seen how the buffers are allocated via __get_free_page() I
> thought so too. However, as it previously read
> 
> 	if (aad_size < PAGE_SIZE)
> 		memset(assoc, 0xff, aad_size);
> 	else {
> 
> my intention was simply to make the modification so that the bug is
> addressed without introducing an additional change.

I fully agree with you. I was just curious about the comparison here.

Best regards,
Marek Vasut

Re: [PATCH 0/3] Cleanup ressource leaks in test_aead_speed()

[Herbert Xu]

On Mon, Apr 21, 2014 at 08:44:39PM +0200, Christian Engelmayer wrote:
> This is a cleanup of Coverity ressource leak findings for the quick & dirty
> crypto testing module crypto/tcrypt.c.
> 
> All 3 changesets address function test_aead_speed() that was introduced in
> 53f52d7a (crypto: tcrypt - Added speed tests for AEAD crypto alogrithms in
> tcrypt test suite)

All applied.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt