| Message ID | 20220607074650.432834-1-niejianglei2021@163.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | security:trusted_tpm2: Fix memory leak in tpm2_key_encode() | expand |
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; > }
"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
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; }
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(-)