[PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so we always kfree() and remember to call va_end()

[PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so we always kfree() and remember to call va_end()

[Jesper Juhl]

In security/keys/trusted_defined.c::TSS_rawhmac() we may leak the storage 
allocated to 'sdesc' if
  data = va_arg(argp, unsigned char *);
results in a NULL 'data' and we then leave the function by returning 
-EINVAL. We also neglect calling va_end(argp) in that case and furthermore 
we neglect va_end(argp) if
  ret = crypto_shash_update(&sdesc->shash, data, dlen);
results in ret being negative and we then jump to the 'out' label.

I believe this patch takes care of these issues. Please review and 
consider for inclusion.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 trusted_defined.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

  compile tested only.

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index 975e9f2..0ec7ab8 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -101,16 +101,18 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
-		if (data == NULL)
-			return -EINVAL;
+		if (data == NULL) {
+			ret = -EINVAL;
+			goto out;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0)
 			goto out;
 	}
-	va_end(argp);
 	if (!ret)
 		ret = crypto_shash_final(&sdesc->shash, digest);
 out:
+	va_end(argp);
 	kfree(sdesc);
 	return ret;
 }



-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so we always kfree() and remember to call va_end()

[David Safford]

On Thu, 2011-01-13 at 21:07 +0100, Jesper Juhl wrote:
> In security/keys/trusted_defined.c::TSS_rawhmac() we may leak the storage 
> allocated to 'sdesc' if
>   data = va_arg(argp, unsigned char *);
> results in a NULL 'data' and we then leave the function by returning 
> -EINVAL. We also neglect calling va_end(argp) in that case and furthermore 
> we neglect va_end(argp) if
>   ret = crypto_shash_update(&sdesc->shash, data, dlen);
> results in ret being negative and we then jump to the 'out' label.
> 
> I believe this patch takes care of these issues. Please review and 
> consider for inclusion.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>

thanks for catching this.

Acked-by: David Safford <safford@watson.ibm.com>

> ---
>  trusted_defined.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>   compile tested only.
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index 975e9f2..0ec7ab8 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -101,16 +101,18 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
>  		if (dlen == 0)
>  			break;
>  		data = va_arg(argp, unsigned char *);
> -		if (data == NULL)
> -			return -EINVAL;
> +		if (data == NULL) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
>  		if (ret < 0)
>  			goto out;
>  	}
> -	va_end(argp);
>  	if (!ret)
>  		ret = crypto_shash_final(&sdesc->shash, digest);
>  out:
> +	va_end(argp);
>  	kfree(sdesc);
>  	return ret;
>  }
> 
> 
> 

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so wealways kfree() and remember to call va_end()

[Tetsuo Handa]

David Safford wrote:
> On Thu, 2011-01-13 at 21:07 +0100, Jesper Juhl wrote:
> > In security/keys/trusted_defined.c::TSS_rawhmac() we may leak the storage 
> > allocated to 'sdesc' if
> >   data = va_arg(argp, unsigned char *);
> > results in a NULL 'data' and we then leave the function by returning 
> > -EINVAL. We also neglect calling va_end(argp) in that case and furthermore 
> > we neglect va_end(argp) if
> >   ret = crypto_shash_update(&sdesc->shash, data, dlen);
> > results in ret being negative and we then jump to the 'out' label.
> > 
> > I believe this patch takes care of these issues. Please review and 
> > consider for inclusion.
> > 
> > Signed-off-by: Jesper Juhl <jj@chaosbits.net>
> 
> thanks for catching this.
> 
> Acked-by: David Safford <safford@watson.ibm.com>

Please wait. That patch is incorrect. I'm making patch now.

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so wealways kfree() and remember to call va_end()

[Tetsuo Handa]

Tetsuo Handa wrote:
> Please wait. That patch is incorrect. I'm making patch now.
I'm doing "git pull" now. Using 2.6.37-git11 instead.

James Morris wrote:
> It's queued in my for-linus branch, waiting to see what happens with 
> http://marc.info/?l=linux-security-module&m=129494927918805&w=3

I think above patch is incorrect because va_end() might be called without
va_start(). C says va_start() without va_end() causes undefined behavior.
I think va_end() without va_start() causes undefined behavior as well.



[PATCH 1/3] Trusted and Encrypted Keys: fix memory leak.

Use "break" rather than "return"/"goto" in order to make sure that
va_end() is always called.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- linux-2.6.37-git11.orig/security/keys/trusted_defined.c
+++ linux-2.6.37-git11/security/keys/trusted_defined.c
@@ -101,11 +101,13 @@ static int TSS_rawhmac(unsigned char *di
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
-		if (data == NULL)
-			return -EINVAL;
+		if (data == NULL) {
+			ret = -EINVAL;
+			break;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0)
-			goto out;
+			break;
 	}
 	va_end(argp);
 	if (!ret)



By the way, TSS_authhmac() has similar code. 

	data = va_arg(argp, unsigned char *);
	ret = crypto_shash_update(&sdesc->shash, data, dlen);

I don't know why we check for NULL in TSS_rawhmac(), but
I think we should check for NULL in TSS_authhmac() as well.



[PATCH 2/3] Trusted and Encrypted Keys: check for NULL.

Check for NULL in TSS_authhmac() as well as TSS_rawhmac().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-2.6.37-git11.orig/security/keys/trusted_defined.c
+++ linux-2.6.37-git11/security/keys/trusted_defined.c
@@ -148,6 +148,11 @@ static int TSS_authhmac(unsigned char *d
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
+		if (!data) {
+			ret = -EINVAL;
+			va_end(argp);
+			goto out;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0) {
 			va_end(argp);



Also, on the assumption that crypto_shash_init()/crypto_shash_update() return 0
on success and negative value otherwise, below cleanup is possible.



[PATCH 3/3] Trusted and Encrypted Keys: avoid goto within va_start()/va_end()

Avoid use of "goto" inside

  va_start();
  for (;;) {

  }
  va_end();

in order to avoid scattering va_end() inside the loop.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |   30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

--- linux-2.6.37-git11.orig/security/keys/trusted_defined.c
+++ linux-2.6.37-git11/security/keys/trusted_defined.c
@@ -150,17 +150,15 @@ static int TSS_authhmac(unsigned char *d
 		data = va_arg(argp, unsigned char *);
 		if (!data) {
 			ret = -EINVAL;
-			va_end(argp);
-			goto out;
+			break;
 		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (!ret)
 		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
 				  paramdigest, TPM_NONCE_SIZE, h1,
@@ -229,13 +227,12 @@ static int TSS_checkhmac1(unsigned char 
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 
@@ -323,13 +320,12 @@ static int TSS_checkhmac2(unsigned char 
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 



Not tested at all. Please review and test.

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so wealways kfree() and remember to call va_end()

[Tetsuo Handa]

Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > Please wait. That patch is incorrect. I'm making patch now.
> I'm doing "git pull" now. Using 2.6.37-git11 instead.
"git pull" completed.
Refreshed using security-testing-2.6#for-linus and compile tested.
Please review and test.

 From 161ec4ee18cc86b41277b9a92a8fb2e732b3662f Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 15 Jan 2011 05:23:53 +0900
Subject: [PATCH 1/3] trusted-keys: another free memory bugfix

TSS_rawhmac() forgot to call va_end()/kfree() when data == NULL and
forgot to call va_end() when crypto_shash_update() < 0.
Fix these bugs by escaping from the loop using "break"
(rather than "return"/"goto") in order to make sure that
va_end()/kfree() are always called.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index 932f868..7b21795 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -101,11 +101,13 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
-		if (data == NULL)
-			return -EINVAL;
+		if (data == NULL) {
+			ret = -EINVAL;
+			break;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0)
-			goto out;
+			break;
 	}
 	va_end(argp);
 	if (!ret)
-- 
1.6.1

 From 06bbcce524ceedb404519cade2c592d66c251595 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 15 Jan 2011 05:40:51 +0900
Subject: [PATCH 2/3] trusted-keys: check for NULL before using it

TSS_rawhmac() checks for data != NULL before using it.
We should do the same thing for TSS_authhmac().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index 7b21795..f7d0677 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -148,6 +148,11 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
+		if (!data) {
+			ret = -EINVAL;
+			va_end(argp);
+			goto out;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0) {
 			va_end(argp);
-- 
1.6.1

 From b4d611a1489da65366ad3e72a00093be76d39fc5 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 15 Jan 2011 05:47:22 +0900
Subject: [PATCH 3/3] trusted-keys: avoid scattring va_end()

We can avoid scattering va_end() within the

  va_start();
  for (;;) {

  }
  va_end();

loop, assuming that crypto_shash_init()/crypto_shash_update() return 0 on
success and negative value otherwise.

Make TSS_authhmac()/TSS_checkhmac1()/TSS_checkhmac2() similar to TSS_rawhmac()
by removing "va_end()/goto" from the loop.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index f7d0677..2836c6d 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -150,17 +150,15 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 		data = va_arg(argp, unsigned char *);
 		if (!data) {
 			ret = -EINVAL;
-			va_end(argp);
-			goto out;
+			break;
 		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (!ret)
 		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
 				  paramdigest, TPM_NONCE_SIZE, h1,
@@ -229,13 +227,12 @@ static int TSS_checkhmac1(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 
@@ -323,13 +320,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 
-- 
1.6.1

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so wealways kfree() and remember to call va_end()

[Jesper Juhl]

On Fri, 14 Jan 2011, Tetsuo Handa wrote:

> Tetsuo Handa wrote:
> > Please wait. That patch is incorrect. I'm making patch now.
> I'm doing "git pull" now. Using 2.6.37-git11 instead.
> 
> James Morris wrote:
> > It's queued in my for-linus branch, waiting to see what happens with 
> > http://marc.info/?l=linux-security-module&m=129494927918805&w=3
> 
> I think above patch is incorrect because va_end() might be called without
> va_start(). C says va_start() without va_end() causes undefined behavior.
> I think va_end() without va_start() causes undefined behavior as well.
> 

I agree. Your patches are better. Thanks.

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

[PATCH 1/3] trusted-keys: another free memory bugfix

[Tetsuo Handa]

Resending in separated mails in case somebody missed that
there was 3 patches in one mail.
----------------------------------------
>From 94e965700f1e401408836d4aa782105483196842 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 17 Jan 2011 09:22:47 +0900
Subject: [PATCH 1/3] trusted-keys: another free memory bugfix

TSS_rawhmac() forgot to call va_end()/kfree() when data == NULL and
forgot to call va_end() when crypto_shash_update() < 0.
Fix these bugs by escaping from the loop using "break"
(rather than "return"/"goto") in order to make sure that
va_end()/kfree() are always called.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index 932f868..7b21795 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -101,11 +101,13 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
-		if (data == NULL)
-			return -EINVAL;
+		if (data == NULL) {
+			ret = -EINVAL;
+			break;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0)
-			goto out;
+			break;
 	}
 	va_end(argp);
 	if (!ret)
-- 
1.7.1

[PATCH 2/3] trusted-keys: check for NULL before using it

[Tetsuo Handa]

>From 8118c3d0d6f2b291d56e2f4475f2aa5156299cf3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 17 Jan 2011 09:25:34 +0900
Subject: [PATCH 2/3] trusted-keys: check for NULL before using it

TSS_rawhmac() checks for data != NULL before using it.
We should do the same thing for TSS_authhmac().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index 7b21795..f7d0677 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -148,6 +148,11 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
+		if (!data) {
+			ret = -EINVAL;
+			va_end(argp);
+			goto out;
+		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
 		if (ret < 0) {
 			va_end(argp);
-- 
1.7.1

[PATCH 3/3] trusted-keys: avoid scattring va_end()

[Tetsuo Handa]

>From 65b41710a476deae2e0899a4df40c02d199a4ee3 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Mon, 17 Jan 2011 09:27:27 +0900
Subject: [PATCH 3/3] trusted-keys: avoid scattring va_end()

We can avoid scattering va_end() within the

  va_start();
  for (;;) {

  }
  va_end();

loop, assuming that crypto_shash_init()/crypto_shash_update() return 0 on
success and negative value otherwise.

Make TSS_authhmac()/TSS_checkhmac1()/TSS_checkhmac2() similar to TSS_rawhmac()
by removing "va_end()/goto" from the loop.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |   30 +++++++++++++-----------------
 1 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index f7d0677..2836c6d 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -150,17 +150,15 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 		data = va_arg(argp, unsigned char *);
 		if (!data) {
 			ret = -EINVAL;
-			va_end(argp);
-			goto out;
+			break;
 		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (!ret)
 		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
 				  paramdigest, TPM_NONCE_SIZE, h1,
@@ -229,13 +227,12 @@ static int TSS_checkhmac1(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 
@@ -323,13 +320,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
-	ret = crypto_shash_final(&sdesc->shash, paramdigest);
+	if (!ret)
+		ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
 
-- 
1.7.1

Re: [PATCH 3/3] trusted-keys: avoid scattring va_end()

[David Howells]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> From 65b41710a476deae2e0899a4df40c02d199a4ee3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:27:27 +0900
> Subject: [PATCH 3/3] trusted-keys: avoid scattring va_end()
> 
> We can avoid scattering va_end() within the
> 
>   va_start();
>   for (;;) {
> 
>   }
>   va_end();
> 
> loop, assuming that crypto_shash_init()/crypto_shash_update() return 0 on
> success and negative value otherwise.
> 
> Make TSS_authhmac()/TSS_checkhmac1()/TSS_checkhmac2() similar to TSS_rawhmac()
> by removing "va_end()/goto" from the loop.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 3/3] trusted-keys: avoid scattring va_end()

[Jesper Juhl]

On Mon, 17 Jan 2011, Tetsuo Handa wrote:

> >From 65b41710a476deae2e0899a4df40c02d199a4ee3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:27:27 +0900
> Subject: [PATCH 3/3] trusted-keys: avoid scattring va_end()
> 
> We can avoid scattering va_end() within the
> 
>   va_start();
>   for (;;) {
> 
>   }
>   va_end();
> 
> loop, assuming that crypto_shash_init()/crypto_shash_update() return 0 on
> success and negative value otherwise.
> 
> Make TSS_authhmac()/TSS_checkhmac1()/TSS_checkhmac2() similar to TSS_rawhmac()
> by removing "va_end()/goto" from the loop.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/keys/trusted_defined.c |   30 +++++++++++++-----------------
>  1 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index f7d0677..2836c6d 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -150,17 +150,15 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
>  		data = va_arg(argp, unsigned char *);
>  		if (!data) {
>  			ret = -EINVAL;
> -			va_end(argp);
> -			goto out;
> +			break;
>  		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (!ret)
>  		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
>  				  paramdigest, TPM_NONCE_SIZE, h1,
> @@ -229,13 +227,12 @@ static int TSS_checkhmac1(unsigned char *buffer,
>  			break;
>  		dpos = va_arg(argp, unsigned int);
>  		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (ret < 0)
>  		goto out;
>  
> @@ -323,13 +320,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
>  			break;
>  		dpos = va_arg(argp, unsigned int);
>  		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (ret < 0)
>  		goto out;
>  
> 
Looks good to me...

Reviewed-by: Jesper Juhl <jj@chaosbits.net>

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 3/3] trusted-keys: avoid scattring va_end()

[Mimi Zohar]

On Mon, 2011-01-17 at 09:44 +0900, Tetsuo Handa wrote:
> From 65b41710a476deae2e0899a4df40c02d199a4ee3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:27:27 +0900
> Subject: [PATCH 3/3] trusted-keys: avoid scattring va_end()
> 
> We can avoid scattering va_end() within the
> 
>   va_start();
>   for (;;) {
> 
>   }
>   va_end();
> 
> loop, assuming that crypto_shash_init()/crypto_shash_update() return 0 on
> success and negative value otherwise.
> 
> Make TSS_authhmac()/TSS_checkhmac1()/TSS_checkhmac2() similar to TSS_rawhmac()
> by removing "va_end()/goto" from the loop.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

The patch looks good.  Would you mind making the one change below?

Acked-by: Mimi Zohar <zohar@us.ibm.com>

> ---
>  security/keys/trusted_defined.c |   30 +++++++++++++-----------------
>  1 files changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index f7d0677..2836c6d 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -150,17 +150,15 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
>  		data = va_arg(argp, unsigned char *);
>  		if (!data) {
>  			ret = -EINVAL;
> -			va_end(argp);
> -			goto out;
> +			break;
>  		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (!ret)

Change the existing '(!ret)' to '(ret < 0)', like the rest of the code?
It's not wrong, but ....

>  		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
>  				  paramdigest, TPM_NONCE_SIZE, h1,
> @@ -229,13 +227,12 @@ static int TSS_checkhmac1(unsigned char *buffer,
>  			break;
>  		dpos = va_arg(argp, unsigned int);
>  		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (ret < 0)
>  		goto out;
> 
> @@ -323,13 +320,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
>  			break;
>  		dpos = va_arg(argp, unsigned int);
>  		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
> -		if (ret < 0) {
> -			va_end(argp);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			break;
>  	}
>  	va_end(argp);
> -	ret = crypto_shash_final(&sdesc->shash, paramdigest);
> +	if (!ret)
> +		ret = crypto_shash_final(&sdesc->shash, paramdigest);
>  	if (ret < 0)
>  		goto out;
> 

[PATCH 3/3] trusted-keys: small cleanup

[Tetsuo Handa]

Mimi Zohar wrote:
> Change the existing '(!ret)' to '(ret < 0)', like the rest of the code?
> It's not wrong, but ....

Something like this?
----------------------------------------
>From 9793efa6fbef62d9e85a06c329761c081ff804c5 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Tue, 18 Jan 2011 10:14:32 +0900
Subject: [PATCH 3/3] trusted-keys: small cleanup

We can avoid scattering va_end() by changing from

  initialize();
  va_start();
  for (;;) {
    if (error condition) {
      va_end();
      goto out;
    }
  }
  va_end();
  if (error condition)
    goto out;
  finalize();
 out:

to

  initialize();
  va_start();
  for (;;) {
    if (error condition)
      break;
  }
  va_end();
  if (error condition)
    goto out;
  finalize();
 out:

.

Also, use

  if (ret < 0)
    goto out;

rather than

  if (!ret)
    ret = do_something();

to clarify error condition.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/keys/trusted_defined.c |   43 ++++++++++++++++++++-------------------
 1 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
index f7d0677..bc04de1 100644
--- a/security/keys/trusted_defined.c
+++ b/security/keys/trusted_defined.c
@@ -101,7 +101,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 		if (dlen == 0)
 			break;
 		data = va_arg(argp, unsigned char *);
-		if (data == NULL) {
+		if (!data) {
 			ret = -EINVAL;
 			break;
 		}
@@ -110,8 +110,9 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 			break;
 	}
 	va_end(argp);
-	if (!ret)
-		ret = crypto_shash_final(&sdesc->shash, digest);
+	if (ret < 0)
+		goto out;
+	ret = crypto_shash_final(&sdesc->shash, digest);
 out:
 	kfree(sdesc);
 	return ret;
@@ -150,21 +151,21 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 		data = va_arg(argp, unsigned char *);
 		if (!data) {
 			ret = -EINVAL;
-			va_end(argp);
-			goto out;
+			break;
 		}
 		ret = crypto_shash_update(&sdesc->shash, data, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
+	if (ret < 0)
+		goto out;
 	ret = crypto_shash_final(&sdesc->shash, paramdigest);
-	if (!ret)
-		ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
-				  paramdigest, TPM_NONCE_SIZE, h1,
-				  TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
+	if (ret < 0)
+		goto out;
+	ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
+			  paramdigest, TPM_NONCE_SIZE, h1,
+			  TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
 out:
 	kfree(sdesc);
 	return ret;
@@ -229,12 +230,12 @@ static int TSS_checkhmac1(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
+	if (ret < 0)
+		goto out;
 	ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
@@ -323,12 +324,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
 			break;
 		dpos = va_arg(argp, unsigned int);
 		ret = crypto_shash_update(&sdesc->shash, buffer + dpos, dlen);
-		if (ret < 0) {
-			va_end(argp);
-			goto out;
-		}
+		if (ret < 0)
+			break;
 	}
 	va_end(argp);
+	if (ret < 0)
+		goto out;
 	ret = crypto_shash_final(&sdesc->shash, paramdigest);
 	if (ret < 0)
 		goto out;
-- 
1.7.1

Re: [PATCH 3/3] trusted-keys: small cleanup

[Mimi Zohar]

On Tue, 2011-01-18 at 10:39 +0900, Tetsuo Handa wrote:
> Mimi Zohar wrote:
> > Change the existing '(!ret)' to '(ret < 0)', like the rest of the code?
> > It's not wrong, but ....
> 
> Something like this?

Actually, I was only asking for a one line change in the patch.

> >       va_end(argp);
> > -     ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > +     if (!ret)
> > +             ret = crypto_shash_final(&sdesc->shash, paramdigest);
> >       if (!ret)

Change the second '(!ret)' here, the crypto_shash_file() return code
test, from '(!ret)' to '(ret < 0)', like the other crypto_shash_file()
tests.

Thanks!

Mimi

Re: [PATCH 3/3] trusted-keys: small cleanup

[Tetsuo Handa]

Mimi Zohar wrote:
> > >       va_end(argp);
> > > -     ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > > +     if (!ret)
> > > +             ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > >       if (!ret)
> 
> Change the second '(!ret)' here, the crypto_shash_file() return code
> test, from '(!ret)' to '(ret < 0)', like the other crypto_shash_file()
> tests.

Did you mean changing from

	if (!ret)
		ret = crypto_shash_final(&sdesc->shash, paramdigest);

to

	if (ret < 0)
		ret = crypto_shash_final(&sdesc->shash, paramdigest);

(i.e. invert the condition)?

I'm confused. Would you make the patch by yourself?

Re: [PATCH 3/3] trusted-keys: small cleanup

[Mimi Zohar]

On Tue, 2011-01-18 at 20:03 +0900, Tetsuo Handa wrote:
> Mimi Zohar wrote:
> > > >       va_end(argp);
> > > > -     ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > > > +     if (!ret)
> > > > +             ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > > >       if (!ret)
> > 
> > Change the second '(!ret)' here, the crypto_shash_file() return code
> > test, from '(!ret)' to '(ret < 0)', like the other crypto_shash_file()
> > tests.
> 
> Did you mean changing from
> 
> 	if (!ret)
> 		ret = crypto_shash_final(&sdesc->shash, paramdigest);
> 
> to
> 
> 	if (ret < 0)
> 		ret = crypto_shash_final(&sdesc->shash, paramdigest);
> 
> (i.e. invert the condition)?

Wrong '(!ret)'.  Instead of:
        va_end(argp);
        if (!ret)
                ret = crypto_shash_final(&sdesc->shash, paramdigest);
        if (!ret)

do:
        va_end(argp);
        if (!ret)
                ret = crypto_shash_final(&sdesc->shash, paramdigest);
        if (ret < 0)

> I'm confused. Would you make the patch by yourself?

It's only for consistency, not that important.

thanks,

Mimi

Re: [PATCH 3/3] trusted-keys: small cleanup

[Mimi Zohar]

On Tue, 2011-01-18 at 06:28 -0500, Mimi Zohar wrote:
> On Tue, 2011-01-18 at 20:03 +0900, Tetsuo Handa wrote:
> > Mimi Zohar wrote:
> > > > >       va_end(argp);
> > > > > -     ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > > > > +     if (!ret)
> > > > > +             ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > > > >       if (!ret)
> > > 
> > > Change the second '(!ret)' here, the crypto_shash_file() return code
> > > test, from '(!ret)' to '(ret < 0)', like the other crypto_shash_file()
> > > tests.
> > 
> > Did you mean changing from
> > 
> > 	if (!ret)
> > 		ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > 
> > to
> > 
> > 	if (ret < 0)
> > 		ret = crypto_shash_final(&sdesc->shash, paramdigest);
> > 
> > (i.e. invert the condition)?
> 
> Wrong '(!ret)'.  Instead of:
>         va_end(argp);
>         if (!ret)
>                 ret = crypto_shash_final(&sdesc->shash, paramdigest);
>         if (!ret)
> 
> do:
>         va_end(argp);
>         if (!ret)
>                 ret = crypto_shash_final(&sdesc->shash, paramdigest);
>         if (ret < 0)
> 
> > I'm confused. Would you make the patch by yourself?
> 
> It's only for consistency, not that important.
> 
> thanks,
> 
> Mimi

James, the original patch is fine as is.

thanks,

Mimi

Re: [PATCH 2/3] trusted-keys: check for NULL before using it

[David Howells]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> From 8118c3d0d6f2b291d56e2f4475f2aa5156299cf3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:25:34 +0900
> Subject: [PATCH 2/3] trusted-keys: check for NULL before using it
> 
> TSS_rawhmac() checks for data != NULL before using it.
> We should do the same thing for TSS_authhmac().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 2/3] trusted-keys: check for NULL before using it

[Jesper Juhl]

On Mon, 17 Jan 2011, Tetsuo Handa wrote:

> >From 8118c3d0d6f2b291d56e2f4475f2aa5156299cf3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:25:34 +0900
> Subject: [PATCH 2/3] trusted-keys: check for NULL before using it
> 
> TSS_rawhmac() checks for data != NULL before using it.
> We should do the same thing for TSS_authhmac().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/keys/trusted_defined.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index 7b21795..f7d0677 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -148,6 +148,11 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
>  		if (dlen == 0)
>  			break;
>  		data = va_arg(argp, unsigned char *);
> +		if (!data) {
> +			ret = -EINVAL;
> +			va_end(argp);
> +			goto out;
> +		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
>  		if (ret < 0) {
>  			va_end(argp);
> 
Looks good to me..

Reviewed-by: Jesper Juhl <jj@chaosbits.net>


-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 2/3] trusted-keys: check for NULL before using it

[Mimi Zohar]

On Mon, 2011-01-17 at 09:41 +0900, Tetsuo Handa wrote:
> From 8118c3d0d6f2b291d56e2f4475f2aa5156299cf3 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:25:34 +0900
> Subject: [PATCH 2/3] trusted-keys: check for NULL before using it
> 
> TSS_rawhmac() checks for data != NULL before using it.
> We should do the same thing for TSS_authhmac().
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Mimi Zohar <zohar@us.ibm.com>

> ---
>  security/keys/trusted_defined.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index 7b21795..f7d0677 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -148,6 +148,11 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
>  		if (dlen == 0)
>  			break;
>  		data = va_arg(argp, unsigned char *);
> +		if (!data) {
> +			ret = -EINVAL;
> +			va_end(argp);
> +			goto out;
> +		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
>  		if (ret < 0) {
>  			va_end(argp);

Re: [PATCH 1/3] trusted-keys: another free memory bugfix

[David Howells]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> From 94e965700f1e401408836d4aa782105483196842 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:22:47 +0900
> Subject: [PATCH 1/3] trusted-keys: another free memory bugfix
> 
> TSS_rawhmac() forgot to call va_end()/kfree() when data == NULL and
> forgot to call va_end() when crypto_shash_update() < 0.
> Fix these bugs by escaping from the loop using "break"
> (rather than "return"/"goto") in order to make sure that
> va_end()/kfree() are always called.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 1/3] trusted-keys: another free memory bugfix

[Jesper Juhl]

On Mon, 17 Jan 2011, Tetsuo Handa wrote:

> Resending in separated mails in case somebody missed that
> there was 3 patches in one mail.
> ----------------------------------------
> >From 94e965700f1e401408836d4aa782105483196842 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:22:47 +0900
> Subject: [PATCH 1/3] trusted-keys: another free memory bugfix
> 
> TSS_rawhmac() forgot to call va_end()/kfree() when data == NULL and
> forgot to call va_end() when crypto_shash_update() < 0.
> Fix these bugs by escaping from the loop using "break"
> (rather than "return"/"goto") in order to make sure that
> va_end()/kfree() are always called.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  security/keys/trusted_defined.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index 932f868..7b21795 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -101,11 +101,13 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
>  		if (dlen == 0)
>  			break;
>  		data = va_arg(argp, unsigned char *);
> -		if (data == NULL)
> -			return -EINVAL;
> +		if (data == NULL) {
> +			ret = -EINVAL;
> +			break;
> +		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
>  		if (ret < 0)
> -			goto out;
> +			break;
>  	}
>  	va_end(argp);
>  	if (!ret)
> 

Looks good to me.

Reviewed-by: Jesper Juhl <jj@chaosbits.net>

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 1/3] trusted-keys: another free memory bugfix

[Mimi Zohar]

On Mon, 2011-01-17 at 09:39 +0900, Tetsuo Handa wrote:
> Resending in separated mails in case somebody missed that
> there was 3 patches in one mail.
> ----------------------------------------
> From 94e965700f1e401408836d4aa782105483196842 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Mon, 17 Jan 2011 09:22:47 +0900
> Subject: [PATCH 1/3] trusted-keys: another free memory bugfix
> 
> TSS_rawhmac() forgot to call va_end()/kfree() when data == NULL and
> forgot to call va_end() when crypto_shash_update() < 0.
> Fix these bugs by escaping from the loop using "break"
> (rather than "return"/"goto") in order to make sure that
> va_end()/kfree() are always called.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Acked-by: Mimi Zohar <zohar@us.ibm.com>

> ---
>  security/keys/trusted_defined.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> index 932f868..7b21795 100644
> --- a/security/keys/trusted_defined.c
> +++ b/security/keys/trusted_defined.c
> @@ -101,11 +101,13 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
>  		if (dlen == 0)
>  			break;
>  		data = va_arg(argp, unsigned char *);
> -		if (data == NULL)
> -			return -EINVAL;
> +		if (data == NULL) {
> +			ret = -EINVAL;
> +			break;
> +		}
>  		ret = crypto_shash_update(&sdesc->shash, data, dlen);
>  		if (ret < 0)
> -			goto out;
> +			break;
>  	}
>  	va_end(argp);
>  	if (!ret)

Re: [PATCH 1/3] trusted-keys: another free memory bugfix

[James Morris]

On Mon, 17 Jan 2011, Tetsuo Handa wrote:

> Resending in separated mails in case somebody missed that
> there was 3 patches in one mail.

All applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#for-linus


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Trusted and Encrypted Keys: fix up TSS_rawhmac() so we always kfree() and remember to call va_end()

[David Howells]

Jesper Juhl <jj@chaosbits.net> wrote:

> In security/keys/trusted_defined.c::TSS_rawhmac() we may leak the storage 
> allocated to 'sdesc' if
>   data = va_arg(argp, unsigned char *);
> results in a NULL 'data' and we then leave the function by returning 
> -EINVAL. We also neglect calling va_end(argp) in that case and furthermore 
> we neglect va_end(argp) if
>   ret = crypto_shash_update(&sdesc->shash, data, dlen);
> results in ret being negative and we then jump to the 'out' label.
> 
> I believe this patch takes care of these issues. Please review and 
> consider for inclusion.
> 
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>

Acked-by: David Howells <dhowells@redhat.com>