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 <[email protected]>
---
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
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 <[email protected]>
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
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 <[email protected]>
> ---
> 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 |
"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 <[email protected]>
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