* 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