From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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 <[email protected]> wrote:
>
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
-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 <[email protected]>
> ---
> 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
>
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
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
>
On Fri, Jul 23, 2021 at 06:21:21PM +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Probably makes sense (for me) to add also
Cc: [email protected]
?
/Jarkko