| Message ID | 20210726114431.18042-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] security: keys: trusted: Fix memory leaks on allocated blob | expand |
On Mon, Jul 26, 2021 at 12:44:31PM +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") > Acked-by: Sumit Garg <sumit.garg@linaro.org> > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > V2: Add a couple more leaky return path fixes as noted by Sumit Garg > Add the if (blob != payload->blob) check on the kfree as > noted by Dan Carpenter > > --- > security/keys/trusted-keys/trusted_tpm2.c | 39 ++++++++++++++++------- > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c > index 0165da386289..a2cfdfdf17fa 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 */ > @@ -406,12 +414,14 @@ static int tpm2_load_cmd(struct tpm_chip *chip, > payload->migratable = 1; > > blob_len = private_len + public_len + 4; > - if (blob_len > payload->blob_len) > - return -E2BIG; > + if (blob_len > payload->blob_len) { > + rc = -E2BIG; > + goto err; > + } > > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD); > if (rc) > - return rc; > + goto err; > > tpm_buf_append_u32(&buf, options->keyhandle); > tpm2_buf_append_auth(&buf, TPM2_RS_PW, > @@ -441,6 +451,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip, > rc = -EPERM; > > return rc; > + > +err: > + if (blob != payload->blob) > + kfree(blob); > + return rc; > } > > /** > -- > 2.31.1 > > Just denoting that I saw this, so just response to my other email, and I'll use this one. /Jarkko
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index 0165da386289..a2cfdfdf17fa 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 */ @@ -406,12 +414,14 @@ static int tpm2_load_cmd(struct tpm_chip *chip, payload->migratable = 1; blob_len = private_len + public_len + 4; - if (blob_len > payload->blob_len) - return -E2BIG; + if (blob_len > payload->blob_len) { + rc = -E2BIG; + goto err; + } rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD); if (rc) - return rc; + goto err; tpm_buf_append_u32(&buf, options->keyhandle); tpm2_buf_append_auth(&buf, TPM2_RS_PW, @@ -441,6 +451,11 @@ static int tpm2_load_cmd(struct tpm_chip *chip, rc = -EPERM; return rc; + +err: + if (blob != payload->blob) + kfree(blob); + return rc; } /**