[PATCH] security: keys: trusted: Fix memory leaks on allocated blob

From: Colin Ian King <redacted>

There are several error return paths that don't kfree the allocated
blob, leading to memory leaks. Ensure blob is initialized to null as
some of the error return paths in function tpm2_key_decode do not
change blob. Add an error return path to kfree blob and use this on
the current leaky returns.

Addresses-Coverity: ("Resource leak")
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Colin Ian King <redacted>
---
 security/keys/trusted-keys/trusted_tpm2.c | 30 ++++++++++++++++-------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..930c67f98611 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c

@@ -366,7 +366,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	unsigned int private_len;
 	unsigned int public_len;
 	unsigned int blob_len;
-	u8 *blob, *pub;
+	u8 *blob = NULL, *pub;
 	int rc;
 	u32 attrs;
 

@@ -378,22 +378,30 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	}
 
 	/* new format carries keyhandle but old format doesn't */
-	if (!options->keyhandle)
-		return -EINVAL;
+	if (!options->keyhandle) {
+		rc = -EINVAL;
+		goto err;
+	}
 
 	/* must be big enough for at least the two be16 size counts */
-	if (payload->blob_len < 4)
-		return -EINVAL;
+	if (payload->blob_len < 4) {
+		rc = -EINVAL;
+		goto err;
+	}
 
 	private_len = get_unaligned_be16(blob);
 
 	/* must be big enough for following public_len */
-	if (private_len + 2 + 2 > (payload->blob_len))
-		return -E2BIG;
+	if (private_len + 2 + 2 > (payload->blob_len)) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	public_len = get_unaligned_be16(blob + 2 + private_len);
-	if (private_len + 2 + public_len + 2 > payload->blob_len)
-		return -E2BIG;
+	if (private_len + 2 + public_len + 2 > payload->blob_len) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	pub = blob + 2 + private_len + 2;
 	/* key attributes are always at offset 4 */

@@ -441,6 +449,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		rc = -EPERM;
 
 	return rc;
+
+err:
+	kfree(blob);
+	return rc;
 }
 
 /**

-- 
2.31.1

Hi Colin,

On Fri, 23 Jul 2021 at 22:51, Colin King [off-list ref] wrote:

From: Colin Ian King <redacted>

There are several error return paths that don't kfree the allocated
blob, leading to memory leaks. Ensure blob is initialized to null as
some of the error return paths in function tpm2_key_decode do not
change blob. Add an error return path to kfree blob and use this on
the current leaky returns.

It looks like there are still leaky return paths left such as
tpm_buf_init() failure etc. which needs to be fixed as well.

With that addressed, feel free to add:

Acked-by: Sumit Garg <redacted>

-Sumit

On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:

This needs to be:

	if (blob != payload->blob)
		kfree(blob);

Otherwise it leads to a use after free.

+	return rc;
 }

How this is allocated is pretty scary looking!

security/keys/trusted-keys/trusted_tpm2.c
    96  static int tpm2_key_decode(struct trusted_key_payload *payload,
    97                             struct trusted_key_options *options,
    98                             u8 **buf)
    99  {
   100          int ret;
   101          struct tpm2_key_context ctx;
   102          u8 *blob;
   103  
   104          memset(&ctx, 0, sizeof(ctx));
   105  
   106          ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
   107                                 payload->blob_len);
   108          if (ret < 0)
   109                  return ret;

Old form?

   110  
   111          if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
   112                  return -EINVAL;

It's really scary to me that if the lengths are too large for kmalloc()
then we just use "payload->blob".

   113  
   114          blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);

blob is allocated here.

   115          if (!blob)
   116                  return -ENOMEM;
   117  
   118          *buf = blob;
   119          options->keyhandle = ctx.parent;
   120  
   121          memcpy(blob, ctx.priv, ctx.priv_len);
   122          blob += ctx.priv_len;
   123  
   124          memcpy(blob, ctx.pub, ctx.pub_len);
   125  
   126          return 0;
   127  }

[ snip ]

   371          u32 attrs;
   372  
   373          rc = tpm2_key_decode(payload, options, &blob);
   374          if (rc) {
   375                  /* old form */
   376                  blob = payload->blob;
                        ^^^^^^^^^^^^^^^^^^^^

   377                  payload->old_format = 1;
   378          }
   379  

regards,
dan carpenter

On 26/07/2021 09:50, Dan Carpenter wrote:

On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:

quoted

@@ -441,6 +449,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		rc = -EPERM;
 
 	return rc;
+
+err:
+	kfree(blob);

This needs to be:

	if (blob != payload->blob)
		kfree(blob);

Good spot! Thanks Dan.

Otherwise it leads to a use after free.

quoted

+	return rc;
 }

How this is allocated is pretty scary looking!

it is kinda mind bending.

Colin

security/keys/trusted-keys/trusted_tpm2.c
    96  static int tpm2_key_decode(struct trusted_key_payload *payload,
    97                             struct trusted_key_options *options,
    98                             u8 **buf)
    99  {
   100          int ret;
   101          struct tpm2_key_context ctx;
   102          u8 *blob;
   103  
   104          memset(&ctx, 0, sizeof(ctx));
   105  
   106          ret = asn1_ber_decoder(&tpm2key_decoder, &ctx, payload->blob,
   107                                 payload->blob_len);
   108          if (ret < 0)
   109                  return ret;

Old form?

   110  
   111          if (ctx.priv_len + ctx.pub_len > MAX_BLOB_SIZE)
   112                  return -EINVAL;

It's really scary to me that if the lengths are too large for kmalloc()
then we just use "payload->blob".

   113  
   114          blob = kmalloc(ctx.priv_len + ctx.pub_len + 4, GFP_KERNEL);

blob is allocated here.

   115          if (!blob)
   116                  return -ENOMEM;
   117  
   118          *buf = blob;
   119          options->keyhandle = ctx.parent;
   120  
   121          memcpy(blob, ctx.priv, ctx.priv_len);
   122          blob += ctx.priv_len;
   123  
   124          memcpy(blob, ctx.pub, ctx.pub_len);
   125  
   126          return 0;
   127  }

[ snip ]

   371          u32 attrs;
   372  
   373          rc = tpm2_key_decode(payload, options, &blob);
   374          if (rc) {
   375                  /* old form */
   376                  blob = payload->blob;
                        ^^^^^^^^^^^^^^^^^^^^

   377                  payload->old_format = 1;
   378          }
   379  

regards,
dan carpenter

On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:

From: Colin Ian King <redacted>

There are several error return paths that don't kfree the allocated
blob, leading to memory leaks. Ensure blob is initialized to null as
some of the error return paths in function tpm2_key_decode do not
change blob. Add an error return path to kfree blob and use this on
the current leaky returns.

Addresses-Coverity: ("Resource leak")
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Colin Ian King <redacted>

 
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Probably makes sense (for me) to add also

Cc: stable@vger.kernel.org

?

/Jarkko