* [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 related [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.