| Message ID | 20210723172121.156687-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | security: keys: trusted: Fix memory leaks on allocated blob | expand |
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 >
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 <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
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; } /**