linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] security: keys: trusted: Fix memory leaks on allocated blob
@ 2021-07-23 17:21 Colin King
  2021-07-26  5:33 ` Sumit Garg
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Colin King @ 2021-07-23 17:21 UTC (permalink / raw)
  To: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
	James Morris, Serge E . Hallyn, linux-integrity, keyrings,
	linux-security-module
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

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 <colin.king@canonical.com>
---
 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] security: keys: trusted: Fix memory leaks on allocated blob
  2021-07-23 17:21 [PATCH] security: keys: trusted: Fix memory leaks on allocated blob Colin King
@ 2021-07-26  5:33 ` Sumit Garg
  2021-07-26  8:50 ` Dan Carpenter
  2021-07-27  3:05 ` Jarkko Sakkinen
  2 siblings, 0 replies; 5+ messages in thread
From: Sumit Garg @ 2021-07-26  5:33 UTC (permalink / raw)
  To: Colin King
  Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
	James Morris, Serge E . Hallyn, linux-integrity,
	open list:ASYMMETRIC KEYS, open list:SECURITY SUBSYSTEM,
	kernel-janitors, Linux Kernel Mailing List

Hi Colin,

On Fri, 23 Jul 2021 at 22:51, Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> 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 <sumit.garg@linaro.org>

-Sumit

> Addresses-Coverity: ("Resource leak")
> Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] security: keys: trusted: Fix memory leaks on allocated blob
  2021-07-23 17:21 [PATCH] security: keys: trusted: Fix memory leaks on allocated blob Colin King
  2021-07-26  5:33 ` Sumit Garg
@ 2021-07-26  8:50 ` Dan Carpenter
  2021-07-26 10:56   ` Colin Ian King
  2021-07-27  3:05 ` Jarkko Sakkinen
  2 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-07-26  8:50 UTC (permalink / raw)
  To: Colin King
  Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
	James Morris, Serge E . Hallyn, linux-integrity, keyrings,
	linux-security-module, kernel-janitors, linux-kernel

On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:
> @@ -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);

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] security: keys: trusted: Fix memory leaks on allocated blob
  2021-07-26  8:50 ` Dan Carpenter
@ 2021-07-26 10:56   ` Colin Ian King
  0 siblings, 0 replies; 5+ messages in thread
From: Colin Ian King @ 2021-07-26 10:56 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: James Bottomley, Jarkko Sakkinen, Mimi Zohar, David Howells,
	James Morris, Serge E . Hallyn, linux-integrity, keyrings,
	linux-security-module, kernel-janitors, linux-kernel

On 26/07/2021 09:50, Dan Carpenter wrote:
> On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:
>> @@ -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.
> 
>> +	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
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] security: keys: trusted: Fix memory leaks on allocated blob
  2021-07-23 17:21 [PATCH] security: keys: trusted: Fix memory leaks on allocated blob Colin King
  2021-07-26  5:33 ` Sumit Garg
  2021-07-26  8:50 ` Dan Carpenter
@ 2021-07-27  3:05 ` Jarkko Sakkinen
  2 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2021-07-27  3:05 UTC (permalink / raw)
  To: Colin King
  Cc: James Bottomley, Mimi Zohar, David Howells, James Morris,
	Serge E . Hallyn, linux-integrity, keyrings,
	linux-security-module, kernel-janitors, linux-kernel

On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> 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 <colin.king@canonical.com>
 
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Probably makes sense (for me) to add also

Cc: stable@vger.kernel.org

?

/Jarkko

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-27  3:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 17:21 [PATCH] security: keys: trusted: Fix memory leaks on allocated blob Colin King
2021-07-26  5:33 ` Sumit Garg
2021-07-26  8:50 ` Dan Carpenter
2021-07-26 10:56   ` Colin Ian King
2021-07-27  3:05 ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).