commit ee618b4619b72527aaed765f0f0b74072b281159 upstream. [Please apply to 3.18-stable.]
As the previous patch did for encrypted-keys, zero sensitive any potentially sensitive data related to the "trusted" key type before it is freed. Notably, we were not zeroing the tpm_buf structures in which the actual key is stored for TPM seal and unseal, nor were we zeroing the trusted_key_payload in certain error paths.
Cc: Mimi Zohar zohar@linux.vnet.ibm.com Cc: David Safford safford@us.ibm.com Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com Signed-off-by: James Morris james.l.morris@oracle.com --- security/keys/trusted.c | 49 ++++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/security/keys/trusted.c b/security/keys/trusted.c index aeb38f1a12e7..917453895cbc 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -69,7 +69,7 @@ static int TSS_sha1(const unsigned char *data, unsigned int datalen, }
ret = crypto_shash_digest(&sdesc->shash, data, datalen, digest); - kfree(sdesc); + kzfree(sdesc); return ret; }
@@ -113,7 +113,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key, if (!ret) ret = crypto_shash_final(&sdesc->shash, digest); out: - kfree(sdesc); + kzfree(sdesc); return ret; }
@@ -164,7 +164,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key, paramdigest, TPM_NONCE_SIZE, h1, TPM_NONCE_SIZE, h2, 1, &c, 0, 0); out: - kfree(sdesc); + kzfree(sdesc); return ret; }
@@ -245,7 +245,7 @@ static int TSS_checkhmac1(unsigned char *buffer, if (memcmp(testhmac, authdata, SHA1_DIGEST_SIZE)) ret = -EINVAL; out: - kfree(sdesc); + kzfree(sdesc); return ret; }
@@ -346,7 +346,7 @@ static int TSS_checkhmac2(unsigned char *buffer, if (memcmp(testhmac2, authdata2, SHA1_DIGEST_SIZE)) ret = -EINVAL; out: - kfree(sdesc); + kzfree(sdesc); return ret; }
@@ -563,7 +563,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype, *bloblen = storedsize; } out: - kfree(td); + kzfree(td); return ret; }
@@ -677,7 +677,7 @@ static int key_seal(struct trusted_key_payload *p, if (ret < 0) pr_info("trusted_key: srkseal failed (%d)\n", ret);
- kfree(tb); + kzfree(tb); return ret; }
@@ -702,7 +702,7 @@ static int key_unseal(struct trusted_key_payload *p, /* pull migratable flag out of sealed key */ p->migratable = p->key[--p->key_len];
- kfree(tb); + kzfree(tb); return ret; }
@@ -961,12 +961,12 @@ static int trusted_instantiate(struct key *key, if (!ret && options->pcrlock) ret = pcrlock(options->pcrlock); out: - kfree(datablob); - kfree(options); + kzfree(datablob); + kzfree(options); if (!ret) rcu_assign_keypointer(key, payload); else - kfree(payload); + kzfree(payload); return ret; }
@@ -975,8 +975,7 @@ static void trusted_rcu_free(struct rcu_head *rcu) struct trusted_key_payload *p;
p = container_of(rcu, struct trusted_key_payload, rcu); - memset(p->key, 0, p->key_len); - kfree(p); + kzfree(p); }
/* @@ -1018,9 +1017,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ret = datablob_parse(datablob, new_p, new_o); if (ret != Opt_update) { ret = -EINVAL; - kfree(new_p); + kzfree(new_p); goto out; } + /* copy old key values, and reseal with new pcrs */ new_p->migratable = p->migratable; new_p->key_len = p->key_len; @@ -1031,22 +1031,22 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ret = key_seal(new_p, new_o); if (ret < 0) { pr_info("trusted_key: key_seal failed (%d)\n", ret); - kfree(new_p); + kzfree(new_p); goto out; } if (new_o->pcrlock) { ret = pcrlock(new_o->pcrlock); if (ret < 0) { pr_info("trusted_key: pcrlock failed (%d)\n", ret); - kfree(new_p); + kzfree(new_p); goto out; } } rcu_assign_keypointer(key, new_p); call_rcu(&p->rcu, trusted_rcu_free); out: - kfree(datablob); - kfree(new_o); + kzfree(datablob); + kzfree(new_o); return ret; }
@@ -1075,24 +1075,19 @@ static long trusted_read(const struct key *key, char __user *buffer, for (i = 0; i < p->blob_len; i++) bufp = hex_byte_pack(bufp, p->blob[i]); if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { - kfree(ascii_buf); + kzfree(ascii_buf); return -EFAULT; } - kfree(ascii_buf); + kzfree(ascii_buf); return 2 * p->blob_len; }
/* - * trusted_destroy - before freeing the key, clear the decrypted data + * trusted_destroy - clear and free the key's payload */ static void trusted_destroy(struct key *key) { - struct trusted_key_payload *p = key->payload.data; - - if (!p) - return; - memset(p->key, 0, p->key_len); - kfree(key->payload.data); + kzfree(key->payload.data); }
struct key_type key_type_trusted = {
commit a3c812f7cfd80cf51e8f5b7034f7418f6beb56c1 upstream. [Please apply to 3.18-stable.]
When calling keyctl_read() on a key of type "trusted", if the user-supplied buffer was too small, the kernel ignored the buffer length and just wrote past the end of the buffer, potentially corrupting userspace memory. Fix it by instead returning the size required, as per the documentation for keyctl_read().
We also don't even fill the buffer at all in this case, as this is slightly easier to implement than doing a short read, and either behavior appears to be permitted. It also makes it match the behavior of the "encrypted" key type.
Fixes: d00a1c72f7f4 ("keys: add new trusted key-type") Reported-by: Ben Hutchings ben@decadent.org.uk Cc: stable@vger.kernel.org # v2.6.38+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com Reviewed-by: Mimi Zohar zohar@linux.vnet.ibm.com Reviewed-by: James Morris james.l.morris@oracle.com Signed-off-by: James Morris james.l.morris@oracle.com --- security/keys/trusted.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/security/keys/trusted.c b/security/keys/trusted.c index 917453895cbc..1273e22aaa28 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -1065,20 +1065,21 @@ static long trusted_read(const struct key *key, char __user *buffer, p = rcu_dereference_key(key); if (!p) return -EINVAL; - if (!buffer || buflen <= 0) - return 2 * p->blob_len; - ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); - if (!ascii_buf) - return -ENOMEM;
- bufp = ascii_buf; - for (i = 0; i < p->blob_len; i++) - bufp = hex_byte_pack(bufp, p->blob[i]); - if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { + if (buffer && buflen >= 2 * p->blob_len) { + ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL); + if (!ascii_buf) + return -ENOMEM; + + bufp = ascii_buf; + for (i = 0; i < p->blob_len; i++) + bufp = hex_byte_pack(bufp, p->blob[i]); + if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) { + kzfree(ascii_buf); + return -EFAULT; + } kzfree(ascii_buf); - return -EFAULT; } - kzfree(ascii_buf); return 2 * p->blob_len; }
On Fri, Nov 10, 2017 at 11:28:51AM -0800, Eric Biggers wrote:
commit a3c812f7cfd80cf51e8f5b7034f7418f6beb56c1 upstream. [Please apply to 3.18-stable.]
When calling keyctl_read() on a key of type "trusted", if the user-supplied buffer was too small, the kernel ignored the buffer length and just wrote past the end of the buffer, potentially corrupting userspace memory. Fix it by instead returning the size required, as per the documentation for keyctl_read().
We also don't even fill the buffer at all in this case, as this is slightly easier to implement than doing a short read, and either behavior appears to be permitted. It also makes it match the behavior of the "encrypted" key type.
Fixes: d00a1c72f7f4 ("keys: add new trusted key-type") Reported-by: Ben Hutchings ben@decadent.org.uk Cc: stable@vger.kernel.org # v2.6.38+ Signed-off-by: Eric Biggers ebiggers@google.com Signed-off-by: David Howells dhowells@redhat.com Reviewed-by: Mimi Zohar zohar@linux.vnet.ibm.com Reviewed-by: James Morris james.l.morris@oracle.com Signed-off-by: James Morris james.l.morris@oracle.com
security/keys/trusted.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Thanks for both of these, now queued up.
greg k-h
linux-stable-mirror@lists.linaro.org