[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jianglei Nie]

From: Jianglei Nie <niejianglei@gmail.com>

Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
it is never freed through the function, which will lead to a memory
leak.

We should kfree() scratch before the function returns (#2, #3 and #4).

31 static int tpm2_key_encode(struct trusted_key_payload *payload,
32			   struct trusted_key_options *options,
33			   u8 *src, u32 len)
34 {
36	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
        // #1: kmalloc space
37	u8 *work = scratch, *work1;
50	if (!scratch)
51		return -ENOMEM;

56	if (options->blobauth_len == 0) {
60		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
61			return PTR_ERR(w); // #2: missing kfree
63	}

71	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
72		 "BUG: scratch buffer is too small"))
73		return -EINVAL; // #3: missing kfree

  	// #4: missing kfree: scratch is never used afterwards.
82	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
83		return PTR_ERR(work1);

85	return work1 - payload->blob;
86 }

Signed-off-by: Jianglei Nie <niejianglei@gmail.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..99bb8b2409ac 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			kfree(scratch);
 			return PTR_ERR(w);
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
+		 "BUG: scratch buffer is too small")){
+		kfree(scratch);
 		return -EINVAL;
+	}
 
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
@@ -79,6 +83,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
+	kfree(scratch);
 	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
 		return PTR_ERR(work1);
 
-- 
2.25.1

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Ahmad Fatoum]

Hello Jianglei,

On 08.06.22 04:59, Jianglei Nie wrote:
> tpm2_key_encode() allocates a memory chunk from scratch with kmalloc(),
> but it is never freed, which leads to a memory leak. Free the memory
> chunk with kfree() in the return path.

Repeating my question in your implicit v1:
Are you sure, scratch need not be freed in the successful return case?
asn1_encode_sequence() copies bytes out of scratch into payload->blob,
so it looks like the buffer is unused after function return.

Cheers,
Ahmad

> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 0165da386289..dc9efd6c8b14 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  		unsigned char bool[3], *w = bool;
>  		/* tag 0 is emptyAuth */
>  		w = asn1_encode_boolean(w, w + sizeof(bool), true);
> -		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> +		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
> +			kfree(scratch);
>  			return PTR_ERR(w);
> +		}
>  		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
>  	}
>  
> @@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	 * trigger, so if it does there's something nefarious going on
>  	 */
>  	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> -		 "BUG: scratch buffer is too small"))
> +		 "BUG: scratch buffer is too small")) {
> +		kfree(scratch);
>  		return -EINVAL;
> +	}
>  
>  	work = asn1_encode_integer(work, end_work, options->keyhandle);
>  	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
> @@ -79,8 +83,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	work1 = payload->blob;
>  	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
>  				     scratch, work - scratch);
> -	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> +	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
> +		kfree(scratch);
>  		return PTR_ERR(work1);
> +	}
>  
>  	return work1 - payload->blob;
>  }


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jarkko Sakkinen]

The short summary (as mentioned in review):

"KEYS: trusted: Fix memory leak in tpm2_key_encode()"

Also, you should version your patches, and provide a change log.

See: https://www.kernel.org/doc/html/v5.18/process/submitting-patches.html#the-canonical-patch-format

For git format-patch  you can simply supply "-vX" to get
the-canonical-patch-format version included.

On Wed, Jun 08, 2022 at 10:59:38AM +0800, Jianglei Nie wrote:
> tpm2_key_encode() allocates a memory chunk from scratch with kmalloc(),
> but it is never freed, which leads to a memory leak. Free the memory
> chunk with kfree() in the return path.
> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
> ---

Here you can write:

v3:
...
v2:
...

>  security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 0165da386289..dc9efd6c8b14 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  		unsigned char bool[3], *w = bool;
>  		/* tag 0 is emptyAuth */
>  		w = asn1_encode_boolean(w, w + sizeof(bool), true);
> -		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> +		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
> +			kfree(scratch);
>  			return PTR_ERR(w);
> +		}
>  		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
>  	}
>  
> @@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	 * trigger, so if it does there's something nefarious going on
>  	 */
>  	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> -		 "BUG: scratch buffer is too small"))
> +		 "BUG: scratch buffer is too small")) {
> +		kfree(scratch);
>  		return -EINVAL;
> +	}
>  
>  	work = asn1_encode_integer(work, end_work, options->keyhandle);
>  	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
> @@ -79,8 +83,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	work1 = payload->blob;
>  	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
>  				     scratch, work - scratch);
> -	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> +	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
> +		kfree(scratch);
>  		return PTR_ERR(work1);
> +	}
>  
>  	return work1 - payload->blob;
>  }
> -- 
> 2.25.1
> 

BR, Jarkko

[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jianglei Nie]

tpm2_key_encode() allocates a memory chunk from scratch with kmalloc(),
but it is never freed, which leads to a memory leak. Free the memory
chunk with kfree() in the return path.

Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..dc9efd6c8b14 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			kfree(scratch);
 			return PTR_ERR(w);
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
+		 "BUG: scratch buffer is too small")) {
+		kfree(scratch);
 		return -EINVAL;
+	}
 
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
@@ -79,8 +83,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
-	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
+	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
+		kfree(scratch);
 		return PTR_ERR(work1);
+	}
 
 	return work1 - payload->blob;
 }
-- 
2.25.1

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jarkko Sakkinen]

"KEYS: trusted: fix memory leak in tpm2_key_encode()"

On Tue, Jun 07, 2022 at 03:46:50PM +0800, Jianglei Nie wrote:
> The function allocates a memory chunk for scratch by kmalloc(), but
                                        ~~~         ~~ 
                                        from        with

There's more than one function in Linux - maybe you'd rather want
to write: "tpm2_key_encode() allocates ..."

> it is never freed through the function, which leads to a memory leak.

You can just write "it is never freed, which leads to a memory leak."

> Handle those cases with kfree().

"Free the memory chunk with kfree() in the return paths."

> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>

Thank you finding this and providing a fix, it is highly appreciated.
Please don't take the nitpicking with the language personally. Just want
to have it documented in appropriate form.

BR, Jarkko

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Ahmad Fatoum]

Hello Jianglei,

On 07.06.22 09:46, Jianglei Nie wrote:
> The function allocates a memory chunk for scratch by kmalloc(), but
> it is never freed through the function, which leads to a memory leak.
> Handle those cases with kfree().

Thanks for your patch.

Shouldn't you free scratch before successful return too?

I haven't looked too deeply, but it looks like scratch is indeed
scratch space and data written to it are memcpy'd elsewhere before
the function returns and no pointer derived from it survives after
function return.

If this is indeed the case, consider also to switch this to a goto out.

Cheers,
Ahmad
  

> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 0165da386289..dc9efd6c8b14 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  		unsigned char bool[3], *w = bool;
>  		/* tag 0 is emptyAuth */
>  		w = asn1_encode_boolean(w, w + sizeof(bool), true);
> -		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> +		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
> +			kfree(scratch);
>  			return PTR_ERR(w);
> +		}
>  		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
>  	}
>  
> @@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	 * trigger, so if it does there's something nefarious going on
>  	 */
>  	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> -		 "BUG: scratch buffer is too small"))
> +		 "BUG: scratch buffer is too small")) {
> +		kfree(scratch);
>  		return -EINVAL;
> +	}
>  
>  	work = asn1_encode_integer(work, end_work, options->keyhandle);
>  	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
> @@ -79,8 +83,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	work1 = payload->blob;
>  	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
>  				     scratch, work - scratch);
> -	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> +	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
> +		kfree(scratch);
>  		return PTR_ERR(work1);
> +	}
>  
>  	return work1 - payload->blob;
>  }


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jianglei Nie]

The function allocates a memory chunk for scratch by kmalloc(), but
it is never freed through the function, which leads to a memory leak.
Handle those cases with kfree().

Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..dc9efd6c8b14 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			kfree(scratch);
 			return PTR_ERR(w);
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
+		 "BUG: scratch buffer is too small")) {
+		kfree(scratch);
 		return -EINVAL;
+	}
 
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
@@ -79,8 +83,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
-	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
+	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
+		kfree(scratch);
 		return PTR_ERR(work1);
+	}
 
 	return work1 - payload->blob;
 }
-- 
2.25.1

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jarkko Sakkinen]

KEYS: trusted: Fix memory leak in tpm2_key_encode()

On Tue, Dec 21, 2021 at 04:54:04PM +0800, Jianglei Nie wrote:
> Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
> it is never freed through the function, which will lead to a memory
> leak.
> 
> We should kfree() scratch before the function returns (#2, #3 and #4).
> 
> 31 static int tpm2_key_encode(struct trusted_key_payload *payload,
> 32			   struct trusted_key_options *options,
> 33			   u8 *src, u32 len)
> 34 {
> 36	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
>       	// #1: kmalloc space
> 50	if (!scratch)
> 51		return -ENOMEM;
> 
> 56	if (options->blobauth_len == 0) {
> 60		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> 61			return PTR_ERR(w); // #2: missing kfree
> 63	}
> 
> 71	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> 72		 "BUG: scratch buffer is too small"))
> 73		return -EINVAL; // #3: missing kfree
> 
>   	// #4: missing kfree: scratch is never used afterwards.
> 82	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> 83		return PTR_ERR(work1);
> 
> 85	return work1 - payload->blob;
> 86 }
> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>

Please write a proper commit message and not just dump tool output. You
are completely lacking analysis of what the heck you are doing.

E.g. you could just:

"The internal buffer in tpm2_key_encode() is not freed, which leads to a
memory leak. Handle those cases with kfree()."

/Jarkko

[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jianglei Nie]

Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
it is never freed through the function, which will lead to a memory
leak.

We should kfree() scratch before the function returns (#2, #3 and #4).

31 static int tpm2_key_encode(struct trusted_key_payload *payload,
32			   struct trusted_key_options *options,
33			   u8 *src, u32 len)
34 {
36	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
      	// #1: kmalloc space
50	if (!scratch)
51		return -ENOMEM;

56	if (options->blobauth_len == 0) {
60		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
61			return PTR_ERR(w); // #2: missing kfree
63	}

71	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
72		 "BUG: scratch buffer is too small"))
73		return -EINVAL; // #3: missing kfree

  	// #4: missing kfree: scratch is never used afterwards.
82	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
83		return PTR_ERR(work1);

85	return work1 - payload->blob;
86 }

Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..7bb1119b1dea 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			kfree(scratch);
 			return PTR_ERR(w);
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,8 +71,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
+		 "BUG: scratch buffer is too small")) {
+		kfree(scratch);
 		return -EINVAL;
+	}
 
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
@@ -79,6 +83,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
+	kfree(scratch);
 	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
 		return PTR_ERR(work1);
 
-- 
2.25.1

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jarkko Sakkinen]

On Sun, Dec 12, 2021 at 09:54:03PM +0800, Jianglei Nie wrote:
> Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
> it is never freed through the function, which will lead to a memory
> leak.
> 

through the function => in the implementation

Also, "line 36" is a relative to something, right? What is it?

/Jarkko

Re: [PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Serge E. Hallyn]

On Sun, Dec 12, 2021 at 09:54:03PM +0800, Jianglei Nie wrote:
> Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
> it is never freed through the function, which will lead to a memory
> leak.
> 
> We should kfree() scratch before the function returns (#2, #3 and #4).
> 
> 31 static int tpm2_key_encode(struct trusted_key_payload *payload,
> 32			   struct trusted_key_options *options,
> 33			   u8 *src, u32 len)
> 34 {
> 36	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
>       	// #1: kmalloc space
> 37	u8 *work = scratch, *work1;
> 50	if (!scratch)
> 51		return -ENOMEM;
> 
> 56	if (options->blobauth_len == 0) {
> 60		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> 61			return PTR_ERR(w); // #2: missing kfree
> 63	}
> 
> 71	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> 72		 "BUG: scratch buffer is too small"))
> 73		return -EINVAL; // #3: missing kfree
> 
>   	// #4: missing kfree: scratch is never used afterwards.
> 82	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
> 83		return PTR_ERR(work1);
> 
> 85	return work1 - payload->blob;
> 86 }
> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>

I don't know that we need to keep the line by line recap in
the full git log, but it def looks correct:

Reviewed-by: Serge Hallyn <serge@hallyn.com>

thanks,
-serge

> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 0165da386289..3408a74c855f 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  		unsigned char bool[3], *w = bool;
>  		/* tag 0 is emptyAuth */
>  		w = asn1_encode_boolean(w, w + sizeof(bool), true);
> -		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
> +		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
> +			kfree(scratch);
>  			return PTR_ERR(w);
> +		}
>  		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
>  	}
>  
> @@ -69,9 +71,12 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  	 * trigger, so if it does there's something nefarious going on
>  	 */
>  	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
> -		 "BUG: scratch buffer is too small"))
> +		 "BUG: scratch buffer is too small")) {
> +		kfree(scratch);
>  		return -EINVAL;
> +	}
>  
> +	kfree(scratch);
>  	work = asn1_encode_integer(work, end_work, options->keyhandle);
>  	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
>  	work = asn1_encode_octet_string(work, end_work, priv, priv_len);
> -- 
> 2.25.1

[PATCH] security:trusted_tpm2: Fix memory leak in tpm2_key_encode()

[Jianglei Nie]

Line 36 (#1) allocates a memory chunk for scratch by kmalloc(), but
it is never freed through the function, which will lead to a memory
leak.

We should kfree() scratch before the function returns (#2, #3 and #4).

31 static int tpm2_key_encode(struct trusted_key_payload *payload,
32			   struct trusted_key_options *options,
33			   u8 *src, u32 len)
34 {
36	u8 *scratch = kmalloc(SCRATCH_SIZE, GFP_KERNEL);
      	// #1: kmalloc space
37	u8 *work = scratch, *work1;
50	if (!scratch)
51		return -ENOMEM;

56	if (options->blobauth_len == 0) {
60		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
61			return PTR_ERR(w); // #2: missing kfree
63	}

71	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
72		 "BUG: scratch buffer is too small"))
73		return -EINVAL; // #3: missing kfree

  	// #4: missing kfree: scratch is never used afterwards.
82	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
83		return PTR_ERR(work1);

85	return work1 - payload->blob;
86 }

Signed-off-by: Jianglei Nie <niejianglei2021@163.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..3408a74c855f 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -57,8 +57,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			kfree(scratch);
 			return PTR_ERR(w);
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,9 +71,12 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
+		 "BUG: scratch buffer is too small")) {
+		kfree(scratch);
 		return -EINVAL;
+	}
 
+	kfree(scratch);
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
 	work = asn1_encode_octet_string(work, end_work, priv, priv_len);
-- 
2.25.1