LinuxLists.cc - [PATCH v1.3 0/4] keys: trusted and encrypted keys

[permalink] [raw]

Subject: [PATCH v1.3 2/4] key: add tpm_send command

Add internal kernel tpm_send() command used to seal/unseal keys.

Signed-off-by: David Safford <[email protected]>
Reviewd-by: Mimi Zohar <[email protected]>
Acked-by: Rajiv Andrade <[email protected]>
Acked-by: Serge E. Hallyn <[email protected]>
---
drivers/char/tpm/tpm.c | 17 +++++++++++++++++
include/linux/tpm.h | 3 +++
2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 7c41335..5987d9c 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -780,6 +780,23 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
}
EXPORT_SYMBOL_GPL(tpm_pcr_extend);

+int tpm_send(u32 chip_num, char *cmd, int buflen)
+{
+ struct tpm_chip *chip;
+ int rc;
+
+ chip = tpm_chip_find_get(chip_num);
+ if (chip == NULL)
+ return -ENODEV;
+
+ rc = transmit_cmd(chip, (struct tpm_cmd_t *)cmd, buflen,
+ "attempting tpm_cmd");
+
+ module_put(chip->dev->driver->owner);
+ return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_send);
+
ssize_t tpm_show_pcrs(struct device *dev, struct device_attribute *attr,
char *buf)
{
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index ac5d1c1..a0ecaa9 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -31,6 +31,7 @@

extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
+extern int tpm_send(u32 chip_num, char *cmd, int buflen);
#else
static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
return -ENODEV;
@@ -38,5 +39,7 @@ static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) {
return -ENODEV;
}
+static inline int tpm_send(u32 chip_num, char *cmd, int buflen) {
+ return -ENODEV;
#endif
#endif
--
1.7.2.2

2010-11-10 15:52:21

[permalink] [raw]

Subject: [PATCH v1.3 4/4] keys: add new key-type encrypted

Defines a new kernel key-type called 'encrypted'. Encrypted keys are
kernel generated random numbers, which are encrypted/decrypted with
a 'trusted' symmetric key. Encrypted keys are created/encrypted/decrypted
in the kernel. Userspace only ever sees/stores encrypted blobs.

Changelog:
- allocate derived_buf dynamically to support arbitrary length master key
(fixed by Roberto Sassu)
- wait until late_initcall for crypto libraries to be registered
- cleanup security/Kconfig
- Add missing 'update' keyword (reported/fixed by Roberto Sassu)
- Free epayload on failure to create key (reported/fixed by Roberto Sassu)
- Increase the data size limit (requested by Roberto Sassu)
- Crypto return codes are always 0 on success and negative on failure,
remove unnecessary tests.
- Replaced kzalloc() with kmalloc()

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: David Safford <[email protected]>
Reviewed-by: Roberto Sassu <[email protected]>
---
include/keys/encrypted-type.h | 30 ++
security/Kconfig | 16 +
security/keys/Makefile | 1 +
security/keys/encrypted_defined.c | 816 +++++++++++++++++++++++++++++++++++++
security/keys/encrypted_defined.h | 52 +++
5 files changed, 915 insertions(+), 0 deletions(-)
create mode 100644 include/keys/encrypted-type.h
create mode 100644 security/keys/encrypted_defined.c
create mode 100644 security/keys/encrypted_defined.h

diff --git a/include/keys/encrypted-type.h b/include/keys/encrypted-type.h
new file mode 100644
index 0000000..e2312e0
--- /dev/null
+++ b/include/keys/encrypted-type.h
@@ -0,0 +1,30 @@
+/* encrypted-type.h: encrypted-defined key type
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Author: Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#ifndef _KEYS_ENCRYPTED_TYPE_H
+#define _KEYS_ENCRYPTED_TYPE_H
+
+#include <linux/key.h>
+#include <linux/rcupdate.h>
+
+struct encrypted_key_payload {
+ struct rcu_head rcu; /* RCU destructor */
+ char *master_desc; /* datablob: master key name */
+ char *datalen; /* datablob: decrypted key length */
+ void *iv; /* datablob: iv */
+ void *encrypted_data; /* datablob: encrypted key */
+ unsigned short datablob_len; /* length of datablob */
+ unsigned short decrypted_datalen; /* decrypted data length */
+ char decrypted_data[0]; /* decrypted data + datablob + hmac */
+};
+
+extern struct key_type key_type_encrypted;
+
+#endif /* _KEYS_ENCRYPTED_TYPE_H */
diff --git a/security/Kconfig b/security/Kconfig
index 415422e..a031ebb 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -36,6 +36,22 @@ config TRUSTED_KEYS

If you are unsure as to whether this is required, answer N.

+config ENCRYPTED_KEYS
+ tristate "ENCRYPTED KEYS"
+ depends on KEYS && TRUSTED_KEYS
+ select CRYPTO_AES
+ select CRYPTO_CBC
+ select CRYPTO_SHA256
+ select CRYPTO_RNG
+ help
+ This option provides support for create/encrypting/decrypting keys
+ in the kernel. Encrypted keys are kernel generated random numbers,
+ which are encrypted/decrypted with a 'master' symmetric key. The
+ 'master' key can be either a trusted-key or user-key type.
+ Userspace only ever sees/stores encrypted blobs.
+
+ If you are unsure as to whether this is required, answer N.
+
config KEYS_DEBUG_PROC_KEYS
bool "Enable the /proc/keys file by which keys may be viewed"
depends on KEYS
diff --git a/security/keys/Makefile b/security/keys/Makefile
index fcb1070..6c94105 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -14,6 +14,7 @@ obj-y := \
user_defined.o

obj-$(CONFIG_TRUSTED_KEYS) += trusted_defined.o
+obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted_defined.o
obj-$(CONFIG_KEYS_COMPAT) += compat.o
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSCTL) += sysctl.o
diff --git a/security/keys/encrypted_defined.c b/security/keys/encrypted_defined.c
new file mode 100644
index 0000000..1d41228
--- /dev/null
+++ b/security/keys/encrypted_defined.c
@@ -0,0 +1,816 @@
+/*
+ * Copyright (C) 2010 IBM Corporation
+ *
+ * Author:
+ * Mimi Zohar <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * File: encrypted_defined.c
+ *
+ * Defines a new kernel key-type called 'encrypted'. Encrypted keys
+ * are kernel generated random numbers, which are encrypted/decrypted
+ * using a 'master' key. The 'master' key can either be a trusted-key or
+ * user-key type. Encrypted keys are created/encrypted/decrypted in the
+ * kernel. Userspace ever only sees/stores encrypted blobs.
+ *
+ * keyctl add "encrypted" "name" "NEW master-key-name keylen" ring
+ * keyctl add "encrypted" "name" "LOAD master-key-name keylen hex_blob" ring
+ * keyctl update keyid "UPDATE master-key-name"
+ */
+
+#include <linux/uaccess.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/parser.h>
+#include <linux/string.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/encrypted-type.h>
+#include <linux/key-type.h>
+#include <linux/random.h>
+#include <linux/rcupdate.h>
+#include <linux/scatterlist.h>
+#include <linux/crypto.h>
+#include <crypto/sha.h>
+#include <crypto/aes.h>
+
+#include "encrypted_defined.h"
+
+static char hash_alg[] = "sha256";
+static char hmac_alg[] = "hmac(sha256)";
+static int hash_size = SHA256_DIGEST_SIZE;
+static char blkcipher_alg[] = "cbc(aes)";
+static int ivsize;
+static int blksize;
+static int MAX_DATA_SIZE = 4096;
+static int MIN_DATA_SIZE = 20;
+
+static int aes_get_sizes(int *ivsize, int *blksize)
+{
+ struct crypto_blkcipher *tfm;
+
+ tfm = crypto_alloc_blkcipher(blkcipher_alg, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(tfm)) {
+ pr_err("encrypted_key: failed to alloc_cipher (%ld)\n",
+ PTR_ERR(tfm));
+ return PTR_ERR(tfm);
+ }
+ *ivsize = crypto_blkcipher_ivsize(tfm);
+ *blksize = crypto_blkcipher_blocksize(tfm);
+ crypto_free_blkcipher(tfm);
+ return 0;
+}
+
+enum {
+ Opt_err = -1, Opt_new = 1, Opt_load,
+ Opt_update, Opt_NEW, Opt_LOAD, Opt_UPDATE
+};
+
+static match_table_t key_tokens = {
+ {Opt_new, "new"},
+ {Opt_NEW, "NEW"},
+ {Opt_load, "load"},
+ {Opt_LOAD, "LOAD"},
+ {Opt_update, "update"},
+ {Opt_UPDATE, "UPDATE"},
+ {Opt_err, NULL}
+};
+
+/*
+ * datablob_parse - parse the keyctl data
+ *
+ * datablob format:
+ * NEW <master-key name> <decrypted data length>
+ * LOAD <master-key name> <decrypted data length> <encrypted iv + data>
+ * UPDATE <new-master-key name>
+ *
+ * Tokenizes a copy of the keyctl data, returning a pointer to each token,
+ * which is null terminated.
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, char **master_desc,
+ char **decrypted_datalen, char **hex_encoded_iv,
+ char **hex_encoded_data)
+{
+ substring_t args[MAX_OPT_ARGS];
+ int ret = -EINVAL;
+ int key_cmd;
+ char *p;
+
+ p = strsep(&datablob, " \t");
+ if (!p)
+ return ret;
+ key_cmd = match_token(p, key_tokens, args);
+
+ *master_desc = strsep(&datablob, " \t");
+ if (!*master_desc)
+ goto out;
+
+ if (decrypted_datalen) {
+ *decrypted_datalen = strsep(&datablob, " \t");
+ if (!*decrypted_datalen)
+ goto out;
+ }
+
+ switch (key_cmd) {
+ case Opt_new:
+ case Opt_NEW:
+ if (!decrypted_datalen)
+ break;
+ ret = 0;
+ break;
+ case Opt_load:
+ case Opt_LOAD:
+ if (!decrypted_datalen)
+ break;
+ *hex_encoded_iv = strsep(&datablob, " \t");
+ if (!*hex_encoded_iv)
+ break;
+ *hex_encoded_data = *hex_encoded_iv + (2 * ivsize) + 2;
+ ret = 0;
+ break;
+ case Opt_update:
+ case Opt_UPDATE:
+ if (decrypted_datalen)
+ break;
+ ret = 0;
+ break;
+ case Opt_err:
+ break;
+ }
+out:
+ return ret;
+}
+
+/* datablob_format - format as an ascii string, before copying to userspace */
+static int datablob_format(char __user *buffer,
+ struct encrypted_key_payload *epayload,
+ int asciiblob_len)
+{
+ char *ascii_buf, *bufp;
+ char *iv = (char *)epayload->iv;
+ int ret = 0;
+ int len;
+ int i;
+
+ ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
+ if (!ascii_buf)
+ return -ENOMEM;
+
+ *(ascii_buf + asciiblob_len) = '\0';
+ len = sprintf(ascii_buf, "%s %s ", epayload->master_desc,
+ epayload->datalen);
+
+ /* convert the hex encoded iv, encrypted-data and HMAC to ascii */
+ bufp = &ascii_buf[len];
+ for (i = 0; i < (asciiblob_len - len) / 2; i++)
+ bufp = pack_hex_byte(bufp, iv[i]);
+
+ if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
+ ret = -EFAULT;
+ kfree(ascii_buf);
+ return ret;
+}
+
+/*
+ * request_trusted_key - request the trusted key
+ *
+ * Trusted keys are sealed to PCRs and other metadata. Although userspace
+ * manages both trusted/encrypted key-types, like the encrypted key type
+ * data, trusted key type data is not visible decrypted from userspace.
+ */
+static struct key *request_trusted_key(char *trusted_desc, void **master_key,
+ unsigned int *master_keylen)
+{
+ struct trusted_key_payload *tpayload;
+ struct key *tkey;
+
+ tkey = request_key(&key_type_trusted, trusted_desc, NULL);
+ if (IS_ERR(tkey))
+ goto error;
+
+ tpayload = tkey->payload.data;
+ *master_key = tpayload->key;
+ *master_keylen = tpayload->key_len;
+error:
+ return tkey;
+}
+
+/*
+ * request_user_key - request the user key
+ *
+ * Use a user provided key to encrypt/decrypt an encrypted-key.
+ */
+static struct key *request_user_key(char *master_desc, void **master_key,
+ unsigned int *master_keylen)
+{
+ struct user_key_payload *upayload;
+ struct key *ukey;
+
+ ukey = request_key(&key_type_user, master_desc, NULL);
+ if (IS_ERR(ukey))
+ goto error;
+
+ upayload = ukey->payload.data;
+ *master_key = upayload->data;
+ *master_keylen = (unsigned int)upayload->datalen;
+error:
+ return ukey;
+}
+
+static int init_desc(struct hash_desc *desc, char *alg)
+{
+ int ret;
+
+ desc->tfm = crypto_alloc_hash(alg, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc->tfm)) {
+ pr_info("encrypted_key: failed to load %s transform: %ld\n",
+ alg, PTR_ERR(desc->tfm));
+ ret = PTR_ERR(desc->tfm);
+ return ret;
+ }
+ desc->flags = 0;
+ ret = crypto_hash_init(desc);
+ if (ret)
+ crypto_free_hash(desc->tfm);
+ return ret;
+}
+
+static int calc_hmac(char *digest, char *key, int keylen,
+ char *buf, size_t buflen)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ int ret;
+
+ ret = init_desc(&desc, hmac_alg);
+ if (ret)
+ return ret;
+
+ crypto_hash_setkey(desc.tfm, key, keylen);
+ ret = crypto_hash_init(&desc);
+ if (ret)
+ goto out;
+
+ sg_init_one(sg, buf, buflen);
+ ret = crypto_hash_update(&desc, sg, buflen);
+ if (!ret)
+ ret = crypto_hash_final(&desc, digest);
+out:
+ crypto_free_hash(desc.tfm);
+ return ret;
+}
+
+static int calc_hash(char *digest, void *buf, int buflen)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ int ret;
+
+ ret = init_desc(&desc, hash_alg);
+ if (ret)
+ return ret;
+
+ sg_init_one(sg, buf, buflen);
+ ret = crypto_hash_update(&desc, sg, buflen);
+ if (!ret)
+ ret = crypto_hash_final(&desc, digest);
+ crypto_free_hash(desc.tfm);
+ return ret;
+}
+
+enum derived_key_type { ENC_KEY, AUTH_KEY };
+
+/* Derive authentication/encryption key from trusted key */
+static int get_derived_key(char *derived_key, enum derived_key_type key_type,
+ void *master_key, unsigned int master_keylen)
+{
+ char *derived_buf;
+ unsigned int derived_buf_len;
+ int ret;
+
+ derived_buf_len = strlen("AUTH_KEY") + 1 + master_keylen;
+ if (derived_buf_len < hash_size)
+ derived_buf_len = hash_size;
+
+ derived_buf = kzalloc(derived_buf_len, GFP_KERNEL);
+ if (!derived_buf) {
+ pr_err("encrypted_key: out of memory\n");
+ return -ENOMEM;
+ }
+ if (key_type)
+ strcpy(derived_buf, "AUTH_KEY");
+ else
+ strcpy(derived_buf, "ENC_KEY");
+
+ memcpy(derived_buf + strlen(derived_buf) + 1, master_key,
+ master_keylen);
+ ret = calc_hash(derived_key, derived_buf, derived_buf_len);
+ kfree(derived_buf);
+ return ret;
+}
+
+static int init_blkcipher_desc(struct blkcipher_desc *desc, const void *key,
+ unsigned int key_len, void *iv, int ivsize)
+{
+ int ret;
+
+ desc->tfm = crypto_alloc_blkcipher(blkcipher_alg, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc->tfm)) {
+ pr_err("encrypted_key: failed to load %s transform (%ld)\n",
+ blkcipher_alg, PTR_ERR(desc->tfm));
+ return PTR_ERR(desc->tfm);
+ }
+ desc->flags = 0;
+
+ ret = crypto_blkcipher_setkey(desc->tfm, key, key_len);
+ if (ret) {
+ pr_err("encrypted_key: failed to setkey (%d)\n", ret);
+ crypto_free_blkcipher(desc->tfm);
+ return ret;
+ }
+ crypto_blkcipher_set_iv(desc->tfm, iv, ivsize);
+ return 0;
+}
+
+static struct key *request_master_key(struct encrypted_key_payload *epayload,
+ void **master_key,
+ unsigned int *master_keylen)
+{
+ struct key *mkey;
+
+ mkey = request_trusted_key(epayload->master_desc,
+ master_key, master_keylen);
+ if (IS_ERR(mkey)) {
+ mkey = request_user_key(epayload->master_desc,
+ master_key, master_keylen);
+ if (IS_ERR(mkey)) {
+ pr_info("encrypted_key: trusted/user key %s not found",
+ epayload->master_desc);
+ return mkey;
+ }
+ }
+ dump_master_key(*master_key, *master_keylen);
+ return mkey;
+}
+
+/* Before returning data to userspace, encrypt decrypted data. */
+static int derived_key_encrypt(struct encrypted_key_payload *epayload,
+ void *derived_key, unsigned int derived_keylen)
+{
+ struct scatterlist sg_in[2];
+ struct scatterlist sg_out[1];
+ struct blkcipher_desc desc;
+ unsigned int encrypted_datalen;
+ unsigned int padlen;
+ char pad[16];
+ int ret;
+
+ encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
+ padlen = encrypted_datalen - epayload->decrypted_datalen;
+
+ ret = init_blkcipher_desc(&desc, derived_key, derived_keylen,
+ epayload->iv, ivsize);
+ if (ret)
+ goto out;
+ dump_decrypted_data(epayload);
+
+ memset(pad, 0, sizeof pad);
+ sg_init_table(sg_in, 2);
+ sg_set_buf(&sg_in[0], epayload->decrypted_data,
+ epayload->decrypted_datalen);
+ sg_set_buf(&sg_in[1], pad, padlen);
+
+ sg_init_table(sg_out, 1);
+ sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen);
+
+ ret = crypto_blkcipher_encrypt(&desc, sg_out, sg_in, encrypted_datalen);
+ crypto_free_blkcipher(desc.tfm);
+ if (ret)
+ pr_err("encrypted_key: failed to encrypt (%d)\n", ret);
+ else
+ dump_encrypted_data(epayload, encrypted_datalen);
+out:
+ return ret;
+}
+
+static int datablob_hmac_append(struct encrypted_key_payload *epayload,
+ void *master_key, unsigned int master_keylen)
+{
+ char derived_key[hash_size];
+ char *digest;
+ int ret;
+
+ memset(derived_key, 0, sizeof derived_key);
+ ret = get_derived_key(derived_key, AUTH_KEY, master_key, master_keylen);
+ if (ret)
+ goto out;
+
+ digest = epayload->master_desc + epayload->datablob_len;
+ ret = calc_hmac(digest, derived_key, sizeof derived_key,
+ epayload->master_desc, epayload->datablob_len);
+ if (!ret)
+ dump_hmac(NULL, digest, hash_size);
+out:
+ return ret;
+}
+
+/* verify HMAC before decrypting encrypted key */
+static int datablob_hmac_verify(struct encrypted_key_payload *epayload,
+ void *master_key, unsigned int master_keylen)
+{
+ char derived_key[hash_size];
+ char digest[hash_size];
+ int ret;
+
+ memset(derived_key, 0, sizeof derived_key);
+ ret = get_derived_key(derived_key, AUTH_KEY, master_key, master_keylen);
+ if (ret)
+ goto out;
+
+ memset(digest, 0, sizeof digest);
+ ret = calc_hmac(digest, derived_key, sizeof derived_key,
+ epayload->master_desc, epayload->datablob_len);
+ if (ret)
+ goto out;
+ ret = memcmp(digest, epayload->master_desc + epayload->datablob_len,
+ sizeof digest);
+ if (ret) {
+ dump_hmac("datablob",
+ epayload->master_desc + epayload->datablob_len,
+ hash_size);
+ dump_hmac("calc", digest, hash_size);
+ }
+out:
+ return ret;
+}
+
+static int derived_key_decrypt(struct encrypted_key_payload *epayload,
+ void *derived_key, unsigned int derived_keylen)
+{
+ struct scatterlist sg_in[1];
+ struct scatterlist sg_out[2];
+ struct blkcipher_desc desc;
+ unsigned int encrypted_datalen;
+ char pad[16];
+ int ret;
+
+ encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
+ ret = init_blkcipher_desc(&desc, derived_key, derived_keylen,
+ epayload->iv, ivsize);
+ if (ret)
+ goto out;
+ dump_encrypted_data(epayload, encrypted_datalen);
+
+ memset(pad, 0, sizeof pad);
+ sg_init_table(sg_in, 1);
+ sg_init_table(sg_out, 2);
+ sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen);
+ sg_set_buf(&sg_out[0], epayload->decrypted_data,
+ (unsigned int)epayload->decrypted_datalen);
+ sg_set_buf(&sg_out[1], pad, sizeof pad);
+
+ ret = crypto_blkcipher_decrypt(&desc, sg_out, sg_in, encrypted_datalen);
+ crypto_free_blkcipher(desc.tfm);
+ if (ret)
+ goto out;
+ dump_decrypted_data(epayload);
+out:
+ return ret;
+}
+
+/* Allocate memory for decrypted key and datablob. */
+static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
+ const char *master_desc,
+ char *datalen)
+{
+ struct encrypted_key_payload *epayload = NULL;
+ unsigned short datablob_len;
+ unsigned short decrypted_datalen;
+ size_t encrypted_datalen;
+ long dlen;
+ int ret;
+
+ ret = strict_strtol(datalen, 10, &dlen);
+ if (ret < 0 || dlen < MIN_DATA_SIZE || dlen > MAX_DATA_SIZE)
+ return ERR_PTR(-EINVAL);
+
+ decrypted_datalen = (unsigned short)dlen;
+ encrypted_datalen = roundup(decrypted_datalen, blksize);
+
+ datablob_len = strlen(master_desc) + 1 + strlen(datalen) + 1
+ + ivsize + 1 + encrypted_datalen;
+
+ ret = key_payload_reserve(key, decrypted_datalen + datablob_len
+ + hash_size + 1);
+ if (ret < 0)
+ return ERR_PTR(ret);
+
+ epayload = kzalloc(sizeof(*epayload) + decrypted_datalen +
+ datablob_len + hash_size + 1, GFP_KERNEL);
+ if (!epayload)
+ return ERR_PTR(-ENOMEM);
+
+ epayload->decrypted_datalen = decrypted_datalen;
+ epayload->datablob_len = datablob_len;
+ return epayload;
+}
+
+static int encrypted_key_decrypt(struct encrypted_key_payload *epayload,
+ char *hex_encoded_iv, char *hex_encoded_data)
+{
+ char derived_key[hash_size];
+ struct key *mkey;
+ void *master_key;
+ unsigned int master_keylen;
+ size_t encrypted_datalen;
+ char *hmac;
+ int ret;
+
+ encrypted_datalen = roundup(epayload->decrypted_datalen, blksize);
+ hex2bin(epayload->iv, hex_encoded_iv, ivsize);
+ hex2bin(epayload->encrypted_data, hex_encoded_data, encrypted_datalen);
+
+ hmac = epayload->master_desc + epayload->datablob_len;
+ hex2bin(hmac, hex_encoded_data + (encrypted_datalen * 2), hash_size);
+
+ mkey = request_master_key(epayload, &master_key, &master_keylen);
+ if (IS_ERR(mkey))
+ return PTR_ERR(mkey);
+
+ ret = datablob_hmac_verify(epayload, master_key, master_keylen);
+ if (ret) {
+ pr_err("encrypted_key: bad hmac (%d)\n", ret);
+ goto out;
+ }
+
+ memset(derived_key, 0, sizeof derived_key);
+ ret = get_derived_key(derived_key, ENC_KEY, master_key, master_keylen);
+ if (ret)
+ goto out;
+
+ ret = derived_key_decrypt(epayload, derived_key, sizeof derived_key);
+ if (ret)
+ pr_err("encrypted_key: failed to decrypt key (%d)\n", ret);
+out:
+ key_put(mkey);
+ return ret;
+}
+
+static void __ekey_init(struct encrypted_key_payload *epayload,
+ char *master_desc, char *datalen)
+{
+ epayload->master_desc = epayload->decrypted_data
+ + epayload->decrypted_datalen;
+ epayload->datalen = epayload->master_desc + strlen(master_desc) + 1;
+ epayload->iv = epayload->datalen + strlen(datalen) + 1;
+ epayload->encrypted_data = epayload->iv + ivsize + 1;
+
+ memcpy(epayload->master_desc, master_desc, strlen(master_desc));
+ memcpy(epayload->datalen, datalen, strlen(datalen));
+}
+
+/*
+ * encrypted_init - initialize an encrypted key
+ *
+ * For a new key, use a random number for both the iv and data
+ * itself. For an old key, decrypt the hex encoded data.
+ */
+static int encrypted_init(struct encrypted_key_payload *epayload,
+ char *master_desc, char *datalen,
+ char *hex_encoded_iv, char *hex_encoded_data)
+{
+ int ret = 0;
+
+ __ekey_init(epayload, master_desc, datalen);
+ if (!hex_encoded_data) {
+ get_random_bytes(epayload->iv, ivsize);
+
+ get_random_bytes(epayload->decrypted_data,
+ epayload->decrypted_datalen);
+ } else
+ ret = encrypted_key_decrypt(epayload, hex_encoded_iv,
+ hex_encoded_data);
+ return ret;
+}
+
+/*
+ * encrypted_instantiate - instantiate an encrypted key
+ *
+ * Decrypt an existing encrypted datablob or create a new encrypted key
+ * based on a kernel random number.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int encrypted_instantiate(struct key *key, const void *data,
+ size_t datalen)
+{
+ struct encrypted_key_payload *epayload = NULL;
+ char *datablob = NULL;
+ char *master_desc = NULL;
+ char *decrypted_datalen = NULL;
+ char *hex_encoded_iv = NULL;
+ char *hex_encoded_data = NULL;
+ int ret;
+
+ if (datalen <= 0 || datalen > 32767 || !data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+ *(datablob + datalen) = 0;
+ memcpy(datablob, data, datalen);
+ ret = datablob_parse(datablob, &master_desc, &decrypted_datalen,
+ &hex_encoded_iv, &hex_encoded_data);
+ if (ret < 0)
+ goto out;
+
+ epayload = encrypted_key_alloc(key, master_desc, decrypted_datalen);
+ if (IS_ERR(epayload)) {
+ ret = PTR_ERR(epayload);
+ goto out;
+ }
+ ret = encrypted_init(epayload, master_desc, decrypted_datalen,
+ hex_encoded_iv, hex_encoded_data);
+ if (ret) {
+ kfree(epayload);
+ goto out;
+ }
+
+ rcu_assign_pointer(key->payload.data, epayload);
+out:
+ kfree(datablob);
+ return ret;
+}
+
+static void encrypted_rcu_free(struct rcu_head *rcu)
+{
+ struct encrypted_key_payload *epayload;
+
+ epayload = container_of(rcu, struct encrypted_key_payload, rcu);
+ memset(epayload->decrypted_data, 0, epayload->decrypted_datalen);
+ kfree(epayload);
+}
+
+/*
+ * encrypted_update - update the master key description
+ *
+ * Change the master key description for an existing encrypted key.
+ * The next read will return an encrypted datablob using the new
+ * master key description.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int encrypted_update(struct key *key, const void *data, size_t datalen)
+{
+ struct encrypted_key_payload *epayload = key->payload.data;
+ struct encrypted_key_payload *new_epayload;
+ char *buf;
+ char *new_master_desc = NULL;
+ int ret = 0;
+
+ if (datalen <= 0 || datalen > 32767 || !data)
+ return -EINVAL;
+
+ buf = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ *(buf + datalen) = 0;
+ memcpy(buf, data, datalen);
+ ret = datablob_parse(buf, &new_master_desc, NULL, NULL, NULL);
+ if (ret < 0)
+ goto out;
+
+ new_epayload = encrypted_key_alloc(key, new_master_desc,
+ epayload->datalen);
+ if (IS_ERR(new_epayload)) {
+ ret = PTR_ERR(new_epayload);
+ goto out;
+ }
+
+ __ekey_init(new_epayload, new_master_desc, epayload->datalen);
+
+ memcpy(new_epayload->iv, epayload->iv, ivsize);
+ memcpy(new_epayload->decrypted_data, epayload->decrypted_data,
+ epayload->decrypted_datalen);
+
+ rcu_assign_pointer(key->payload.data, new_epayload);
+ call_rcu(&epayload->rcu, encrypted_rcu_free);
+out:
+ kfree(buf);
+ return ret;
+}
+
+/*
+ * encrypted_read - format and copy the encrypted data to userspace
+ *
+ * The resulting datablob format is:
+ * <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
+ *
+ * On success, return to userspace the encrypted key datablob size.
+ */
+static long encrypted_read(const struct key *key, char __user * buffer,
+ size_t buflen)
+{
+ struct encrypted_key_payload *epayload;
+ struct key *mkey;
+ void *master_key;
+ unsigned int master_keylen;
+ char derived_key[hash_size];
+ int asciiblob_len;
+ int ret;
+
+ epayload = rcu_dereference_protected(key->payload.data,
+ rwsem_is_locked(&((struct key *)key)->sem));
+
+ /* returns the hex encoded iv, encrypted-data, and hmac as ascii */
+ asciiblob_len = epayload->datablob_len + ivsize + 1
+ + roundup(epayload->decrypted_datalen, blksize)
+ + (hash_size * 2);
+
+ if (!buffer || buflen <= 0)
+ return asciiblob_len;
+
+ mkey = request_master_key(epayload, &master_key, &master_keylen);
+ if (IS_ERR(mkey))
+ return PTR_ERR(mkey);
+
+ memset(derived_key, 0, sizeof derived_key);
+ ret = get_derived_key(derived_key, ENC_KEY, master_key, master_keylen);
+ if (ret)
+ goto out;
+
+ ret = derived_key_encrypt(epayload, derived_key, sizeof derived_key);
+ if (ret)
+ goto out;
+
+ ret = datablob_hmac_append(epayload, master_key, master_keylen);
+ if (ret)
+ goto out;
+
+ ret = datablob_format(buffer, epayload, asciiblob_len);
+ if (ret < 0)
+ goto out;
+
+ key_put(mkey);
+ return asciiblob_len;
+out:
+ key_put(mkey);
+ return ret;
+}
+
+/*
+ * encrypted_destroy - before freeing the key, clear the decrypted data
+ *
+ * Before freeing the key, clear the memory containing the descrypted
+ * key data.
+ */
+static void encrypted_destroy(struct key *key)
+{
+ struct encrypted_key_payload *epayload = key->payload.data;
+
+ if (!epayload)
+ return;
+
+ memset(epayload->decrypted_data, 0, epayload->decrypted_datalen);
+ kfree(key->payload.data);
+}
+
+struct key_type key_type_encrypted = {
+ .name = "encrypted",
+ .instantiate = encrypted_instantiate,
+ .update = encrypted_update,
+ .match = user_match,
+ .destroy = encrypted_destroy,
+ .describe = user_describe,
+ .read = encrypted_read,
+};
+EXPORT_SYMBOL_GPL(key_type_encrypted);
+
+static int __init init_encrypted(void)
+{
+ int ret;
+
+ ret = register_key_type(&key_type_encrypted);
+ if (ret < 0)
+ return ret;
+ ret = aes_get_sizes(&ivsize, &blksize);
+ return ret;
+}
+
+static void __exit cleanup_encrypted(void)
+{
+ unregister_key_type(&key_type_encrypted);
+}
+
+late_initcall(init_encrypted);
+module_exit(cleanup_encrypted);
+
+MODULE_LICENSE("GPL");
diff --git a/security/keys/encrypted_defined.h b/security/keys/encrypted_defined.h
new file mode 100644
index 0000000..4e0b6e5
--- /dev/null
+++ b/security/keys/encrypted_defined.h
@@ -0,0 +1,52 @@
+#ifndef __ENCRYPTED_KEY_H
+#define __ENCRYPTED_KEY_H
+
+#define ENCRYPTED_DEBUG 0
+
+#if ENCRYPTED_DEBUG
+static inline void dump_master_key(void *master_key, unsigned int master_keylen)
+{
+ print_hex_dump(KERN_ERR, "master key: ", DUMP_PREFIX_NONE, 32, 1,
+ master_key, (size_t) master_keylen, 0);
+}
+
+static inline void dump_decrypted_data(struct encrypted_key_payload *epayload)
+{
+ print_hex_dump(KERN_ERR, "decrypted data: ", DUMP_PREFIX_NONE, 32, 1,
+ epayload->decrypted_data,
+ epayload->decrypted_datalen, 0);
+}
+
+static inline void dump_encrypted_data(struct encrypted_key_payload *epayload,
+ unsigned int encrypted_datalen)
+{
+ print_hex_dump(KERN_ERR, "encrypted data: ", DUMP_PREFIX_NONE, 32, 1,
+ epayload->encrypted_data, (size_t) encrypted_datalen, 0);
+}
+
+static inline void dump_hmac(char *str, void *digest, unsigned int hmac_size)
+{
+ if (str)
+ pr_info("encrypted_key: %s", str);
+ print_hex_dump(KERN_ERR, "hmac: ", DUMP_PREFIX_NONE, 32, 1, digest,
+ (size_t) hmac_size, 0);
+}
+#else
+static inline void dump_master_key(void *master_key, unsigned int master_keylen)
+{
+}
+
+static inline void dump_decrypted_data(struct encrypted_key_payload *epayload)
+{
+}
+
+static inline void dump_encrypted_data(struct encrypted_key_payload *epayload,
+ unsigned int encrypted_datalen)
+{
+}
+
+static inline void dump_hmac(char *str, void *digest, int hmac_size)
+{
+}
+#endif
+#endif
--
1.7.2.2

2010-11-10 15:52:26

[permalink] [raw]

Subject: [PATCH v1.3 3/4] keys: add new trusted key-type

Defines a new kernel key-type called 'trusted'. Trusted keys are
random number symmetric keys, generated and RSA-sealed by the TPM.
The TPM only unseals the keys, if the boot PCRs and other criteria
match. Userspace can only ever see encrypted blobs.

Based on suggestions by Jason Gunthorpe, several new options have
been added to support additional usages.

The new options are:
migratable= designates that the key may/may not ever be updated
(resealed under a new key, new pcrinfo or new auth.)

pcrlock=n extends the designated PCR 'n' with a random value,
so that a key sealed to that PCR may not be unsealed
again until after a reboot.

keyhandle= specifies the sealing/unsealing key handle.

keyauth= specifies the sealing/unsealing key auth.

blobauth= specifies the sealed data auth.

Implementation of a kernel reserved locality for trusted keys
will be investigated for a possible future extension.

Changelog:
- Add pcrlock CAP_SYS_ADMIN dependency (based on comment by Jason Gunthorpe)
- New options: migratable, pcrlock, keyhandle, keyauth, blobauth (based on
discussions with Jason Gunthorpe)
- Free payload on failure to create key(reported/fixed by Roberto Sassu)
- Updated Kconfig and other descriptions (based on Serge Hallyn's suggestion)
- Replaced kzalloc() with kmalloc() (reported by Serge Hallyn)

Signed-off-by: David Safford <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
---
include/keys/trusted-type.h | 32 ++
security/Kconfig | 15 +
security/keys/Makefile | 1 +
security/keys/trusted_defined.c | 1101 +++++++++++++++++++++++++++++++++++++++
security/keys/trusted_defined.h | 147 ++++++
5 files changed, 1296 insertions(+), 0 deletions(-)
create mode 100644 include/keys/trusted-type.h
create mode 100644 security/keys/trusted_defined.c
create mode 100644 security/keys/trusted_defined.h

diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
new file mode 100644
index 0000000..5c3a158
--- /dev/null
+++ b/include/keys/trusted-type.h
@@ -0,0 +1,32 @@
+/* trusted-type.h: trusted-defined key type
+ *
+ * Copyright (C) 2010 IBM Corporation
+ * Author: David Safford <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#ifndef _KEYS_TRUSTED_TYPE_H
+#define _KEYS_TRUSTED_TYPE_H
+
+#include <linux/key.h>
+#include <linux/rcupdate.h>
+
+#define MIN_KEY_SIZE 32
+#define MAX_KEY_SIZE 128
+#define MAX_BLOB_SIZE 320
+
+struct trusted_key_payload {
+ struct rcu_head rcu; /* RCU destructor */
+ unsigned int key_len;
+ unsigned int blob_len;
+ unsigned char migratable;
+ unsigned char key[MAX_KEY_SIZE+1];
+ unsigned char blob[MAX_BLOB_SIZE];
+};
+
+extern struct key_type key_type_trusted;
+
+#endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/security/Kconfig b/security/Kconfig
index bd72ae6..415422e 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -21,6 +21,21 @@ config KEYS

If you are unsure as to whether this is required, answer N.

+config TRUSTED_KEYS
+ tristate "TRUSTED KEYS"
+ depends on KEYS && TCG_TPM
+ select CRYPTO
+ select CRYPTO_HMAC
+ select CRYPTO_SHA1
+ help
+ This option provides support for creating, sealing, and unsealing
+ keys in the kernel. Trusted keys are random number symmetric keys,
+ generated and RSA-sealed by the TPM. The TPM only unseals the keys,
+ if the boot PCRs and other criteria match. Userspace can only ever
+ see encrypted blobs.
+
+ If you are unsure as to whether this is required, answer N.
+
config KEYS_DEBUG_PROC_KEYS
bool "Enable the /proc/keys file by which keys may be viewed"
depends on KEYS
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 74d5447..fcb1070 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -13,6 +13,7 @@ obj-y := \
request_key_auth.o \
user_defined.o

+obj-$(CONFIG_TRUSTED_KEYS) += trusted_defined.o
obj-$(CONFIG_KEYS_COMPAT) += compat.o
obj-$(CONFIG_PROC_FS) += proc.o
obj-$(CONFIG_SYSCTL) += sysctl.o
diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
new file mode 100644
index 0000000..d45964d
--- /dev/null
+++ b/security/keys/trusted_defined.c
@@ -0,0 +1,1101 @@
+/*
+ * Copyright (C) 2010 IBM Corporation
+ *
+ * Author:
+ * David Safford <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * Defines a new kernel key-type called 'trusted'. Trusted keys are random
+ * number symmetric keys, generated and RSA-sealed by the TPM. The TPM only
+ * unseals the keys, if the boot PCRs and other criteria match. Userspace
+ * can only ever see encrypted blobs.
+ *
+ * By default, trusted keys are sealed with the Storage Root Key (SRK).
+ * The SRK usage authorization value is the well-known value of 20 zeroes.
+ * This can be set at takeownership time with the trouser's utility
+ * "tpm_takeownership -u -z".
+ *
+ * Usage:
+ * keyctl add trusted name "new keylen [options]" ring
+ * keyctl update key "update [options]"
+ * keyctl add trusted name "load hex_blob [pcrlock=pcrnum]" ring
+ * keyctl print keyid
+ *
+ * options:
+ * keyhandle= ascii hex value of sealing key
+ * default 0x40000000 (SRK)
+ * keyauth= ascii hex auth for sealing key
+ * default 0x00... (40 ascii zeros)
+ * blobauth= ascii hex auth for sealed data
+ * default 0x00... (40 ascii zeros)
+ * pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG
+ * (no default - create with genpcrinfo util)
+ * pcrlock= pcr number to be extended to "lock" blob
+ * migratable= 0|1 indicating permission to reseal to new PCR values
+ * default 1 (resealing allowed)
+ *
+ * keyctl print returns an ascii hex copy of the sealed key, which is in
+ * standard TPM_STORED_DATA format.
+ * keys can be 32 - 128 bytes, blob max is 1024 hex ascii characters
+ * binary pcrinfo max is 512 hex ascii characters
+ */
+
+#include <linux/uaccess.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/parser.h>
+#include <linux/string.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <linux/key-type.h>
+#include <linux/random.h>
+#include <linux/rcupdate.h>
+#include <linux/scatterlist.h>
+#include <linux/crypto.h>
+#include <linux/capability.h>
+#include <linux/tpm.h>
+
+#include "trusted_defined.h"
+
+static char hmac_alg[] = "hmac(sha1)";
+static char hash_alg[] = "sha1";
+
+static int init_sha1_desc(struct hash_desc *desc)
+{
+ int rc;
+
+ desc->tfm = crypto_alloc_hash(hash_alg, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc->tfm)) {
+ rc = PTR_ERR(desc->tfm);
+ return rc;
+ }
+ desc->flags = 0;
+ rc = crypto_hash_init(desc);
+ if (rc)
+ crypto_free_hash(desc->tfm);
+ return rc;
+}
+
+static int init_hmac_desc(struct hash_desc *desc, unsigned char *key,
+ int keylen)
+{
+ int rc;
+
+ desc->tfm = crypto_alloc_hash(hmac_alg, 0, CRYPTO_ALG_ASYNC);
+ if (IS_ERR(desc->tfm)) {
+ rc = PTR_ERR(desc->tfm);
+ return rc;
+ }
+ desc->flags = 0;
+ crypto_hash_setkey(desc->tfm, key, keylen);
+ rc = crypto_hash_init(desc);
+ if (rc)
+ crypto_free_hash(desc->tfm);
+ return rc;
+}
+
+static int TSS_sha1(unsigned char *data, int datalen, unsigned char *digest)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ int rc;
+
+ rc = init_sha1_desc(&desc);
+ if (rc != 0)
+ return rc;
+
+ sg_init_one(sg, data, datalen);
+ rc = crypto_hash_update(&desc, sg, datalen);
+ if (rc < 0)
+ goto out;
+ rc = crypto_hash_final(&desc, digest);
+out:
+ crypto_free_hash(desc.tfm);
+ return rc;
+}
+
+static int TSS_rawhmac(unsigned char *digest, unsigned char *key,
+ unsigned int keylen, ...)
+{
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ unsigned int dlen;
+ unsigned char *data;
+ va_list argp;
+ int error;
+
+ error = init_hmac_desc(&desc, key, keylen);
+ if (error)
+ return error;
+
+ va_start(argp, keylen);
+ for (;;) {
+ dlen = (unsigned int)va_arg(argp, unsigned int);
+ if (dlen == 0)
+ break;
+ data = (unsigned char *)va_arg(argp, unsigned char *);
+ if (data == NULL)
+ return -1;
+ sg_init_one(sg, data, dlen);
+ error = crypto_hash_update(&desc, sg, dlen);
+ if (error < 0)
+ break;
+ }
+ error = crypto_hash_final(&desc, digest);
+ crypto_free_hash(desc.tfm);
+
+ va_end(argp);
+ return error;
+}
+
+/*
+ * calculate authorization info fields to send to TPM
+ */
+static uint32_t TSS_authhmac(unsigned char *digest, unsigned char *key,
+ unsigned int keylen, unsigned char *h1,
+ unsigned char *h2, unsigned char h3, ...)
+{
+ unsigned char paramdigest[TPM_HASH_SIZE];
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ unsigned int dlen;
+ unsigned char *data;
+ unsigned char c;
+ int rc;
+
+ va_list argp;
+
+ rc = init_sha1_desc(&desc);
+ if (rc != 0)
+ return rc;
+ c = h3;
+ va_start(argp, h3);
+ for (;;) {
+ dlen = (unsigned int)va_arg(argp, unsigned int);
+ if (dlen == 0)
+ break;
+ data = (unsigned char *)va_arg(argp, unsigned char *);
+ sg_init_one(sg, data, dlen);
+ rc = crypto_hash_update(&desc, sg, dlen);
+ if (rc < 0)
+ break;
+ }
+ va_end(argp);
+ rc = crypto_hash_final(&desc, paramdigest);
+ crypto_free_hash(desc.tfm);
+ TSS_rawhmac(digest, key, keylen, TPM_HASH_SIZE, paramdigest,
+ TPM_NONCE_SIZE, h1, TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
+ return rc;
+}
+
+/*
+ * verify the AUTH1_COMMAND (Seal) result from TPM
+ */
+static uint32_t TSS_checkhmac1(unsigned char *buffer, uint32_t command,
+ unsigned char *ononce, unsigned char *key,
+ unsigned int keylen, ...)
+{
+ uint32_t bufsize;
+ uint16_t tag;
+ uint32_t ordinal;
+ uint32_t result;
+ unsigned char *enonce;
+ unsigned char *continueflag;
+ unsigned char *authdata;
+ unsigned char testhmac[TPM_HASH_SIZE];
+ unsigned char paramdigest[TPM_HASH_SIZE];
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ unsigned int dlen;
+ unsigned int dpos;
+ va_list argp;
+ int rc;
+
+ bufsize = LOAD32(buffer, TPM_SIZE_OFFSET);
+ tag = LOAD16(buffer, 0);
+ ordinal = command;
+ result = LOAD32N(buffer, TPM_RETURN_OFFSET);
+
+ if (tag == TPM_TAG_RSP_COMMAND)
+ return 0;
+ if (tag != TPM_TAG_RSP_AUTH1_COMMAND)
+ return -1;
+ authdata = buffer + bufsize - TPM_HASH_SIZE;
+ continueflag = authdata - 1;
+ enonce = continueflag - TPM_NONCE_SIZE;
+ rc = init_sha1_desc(&desc);
+ if (rc != 0)
+ return rc;
+ sg_init_one(sg, &result, TPM_U32_SIZE);
+ crypto_hash_update(&desc, sg, TPM_U32_SIZE);
+ sg_init_one(sg, &ordinal, TPM_U32_SIZE);
+ crypto_hash_update(&desc, sg, TPM_U32_SIZE);
+ va_start(argp, keylen);
+ for (;;) {
+ dlen = (unsigned int)va_arg(argp, unsigned int);
+ if (dlen == 0)
+ break;
+ dpos = (unsigned int)va_arg(argp, unsigned int);
+ sg_init_one(sg, buffer + dpos, dlen);
+ rc = crypto_hash_update(&desc, sg, dlen);
+ if (rc < 0)
+ break;
+ }
+ va_end(argp);
+ crypto_hash_final(&desc, paramdigest);
+ crypto_free_hash(desc.tfm);
+
+ TSS_rawhmac(testhmac, key, keylen, TPM_HASH_SIZE, paramdigest,
+ TPM_NONCE_SIZE, enonce,
+ TPM_NONCE_SIZE, ononce, 1, continueflag, 0, 0);
+ if (memcmp(testhmac, authdata, TPM_HASH_SIZE) != 0)
+ return -1;
+ return 0;
+}
+
+/*
+ * verify the AUTH2_COMMAND (unseal) result from TPM
+ */
+static uint32_t TSS_checkhmac2(unsigned char *buffer, uint32_t command,
+ unsigned char *ononce, unsigned char *key1,
+ unsigned int keylen1, unsigned char *key2,
+ unsigned int keylen2, ...)
+{
+ uint32_t bufsize;
+ uint16_t tag;
+ uint32_t ordinal;
+ uint32_t result;
+ unsigned char *enonce1;
+ unsigned char *continueflag1;
+ unsigned char *authdata1;
+ unsigned char *enonce2;
+ unsigned char *continueflag2;
+ unsigned char *authdata2;
+ unsigned char testhmac1[TPM_HASH_SIZE];
+ unsigned char testhmac2[TPM_HASH_SIZE];
+ unsigned char paramdigest[TPM_HASH_SIZE];
+ struct hash_desc desc;
+ struct scatterlist sg[1];
+ unsigned int dlen;
+ unsigned int dpos;
+ va_list argp;
+ int rc;
+
+ bufsize = LOAD32(buffer, TPM_SIZE_OFFSET);
+ tag = LOAD16(buffer, 0);
+ ordinal = command;
+ result = LOAD32N(buffer, TPM_RETURN_OFFSET);
+
+ if (tag == TPM_TAG_RSP_COMMAND)
+ return 0;
+ if (tag != TPM_TAG_RSP_AUTH2_COMMAND)
+ return -1;
+ authdata1 = buffer + bufsize - (TPM_HASH_SIZE + 1
+ + TPM_HASH_SIZE + TPM_HASH_SIZE);
+ authdata2 = buffer + bufsize - (TPM_HASH_SIZE);
+ continueflag1 = authdata1 - 1;
+ continueflag2 = authdata2 - 1;
+ enonce1 = continueflag1 - TPM_NONCE_SIZE;
+ enonce2 = continueflag2 - TPM_NONCE_SIZE;
+
+ rc = init_sha1_desc(&desc);
+ if (rc != 0)
+ return rc;
+ sg_init_one(sg, &result, TPM_U32_SIZE);
+ crypto_hash_update(&desc, sg, TPM_U32_SIZE);
+ sg_init_one(sg, &ordinal, TPM_U32_SIZE);
+ crypto_hash_update(&desc, sg, TPM_U32_SIZE);
+
+ va_start(argp, keylen2);
+ for (;;) {
+ dlen = (unsigned int)va_arg(argp, unsigned int);
+ if (dlen == 0)
+ break;
+ dpos = (unsigned int)va_arg(argp, unsigned int);
+ sg_init_one(sg, buffer + dpos, dlen);
+ rc = crypto_hash_update(&desc, sg, dlen);
+ if (rc < 0)
+ break;
+ }
+ crypto_hash_final(&desc, paramdigest);
+ crypto_free_hash(desc.tfm);
+
+ TSS_rawhmac(testhmac1, key1, keylen1, TPM_HASH_SIZE, paramdigest,
+ TPM_NONCE_SIZE, enonce1,
+ TPM_NONCE_SIZE, ononce, 1, continueflag1, 0, 0);
+ TSS_rawhmac(testhmac2, key2, keylen2, TPM_HASH_SIZE, paramdigest,
+ TPM_NONCE_SIZE, enonce2,
+ TPM_NONCE_SIZE, ononce, 1, continueflag2, 0, 0);
+ if (memcmp(testhmac1, authdata1, TPM_HASH_SIZE) != 0)
+ return -1;
+ if (memcmp(testhmac2, authdata2, TPM_HASH_SIZE) != 0)
+ return -1;
+ return 0;
+}
+
+/*
+ * For key specific tpm requests, we will generate and send our
+ * own TPM command packets using the drivers send function.
+ */
+static int trusted_tpm_send(u32 chip_num, unsigned char *cmd, int buflen)
+{
+ int rc;
+
+ dump_tpm_buf(cmd);
+ rc = tpm_send(chip_num, cmd, buflen);
+ dump_tpm_buf(cmd);
+ if (rc > 0)
+ /* Can't return positive return codes values to keyctl */
+ rc = -EPERM;
+ return rc;
+}
+
+/*
+ * get a random value from TPM
+ */
+static int tpm_get_random(struct tpm_buf *tb, unsigned char *buf, uint32_t len)
+{
+ int ret;
+
+ INIT_BUF(tb);
+ store16(tb, TPM_TAG_RQU_COMMAND);
+ store32(tb, TPM_GETRANDOM_SIZE);
+ store32(tb, TPM_ORD_GETRANDOM);
+ store32(tb, len);
+ ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, sizeof tb->data);
+ memcpy(buf, tb->data + TPM_GETRANDOM_RETURN, len);
+
+ return ret;
+}
+
+static int my_get_random(unsigned char *buf, int len)
+{
+ struct tpm_buf *tb;
+ int ret;
+
+ tb = kzalloc(sizeof *tb, GFP_KERNEL);
+ if (!tb)
+ return -ENOMEM;
+ ret = tpm_get_random(tb, buf, len);
+
+ kfree(tb);
+ return ret;
+}
+
+/*
+ * Lock a trusted key, by extending a selected PCR.
+ *
+ * Prevents a trusted key that is sealed to PCRs from being accessed.
+ * This uses the tpm driver's extend function.
+ */
+static int pcrlock(int pcrnum)
+{
+ unsigned char hash[TPM_HASH_SIZE];
+ int ret;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ my_get_random(hash, TPM_HASH_SIZE);
+ ret = tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash);
+ return ret;
+}
+
+/*
+ * Create an object specific authorisation protocol (OSAP) session
+ */
+static int osap(struct tpm_buf *tb, struct osapsess *s,
+ unsigned char *key, uint16_t type, uint32_t handle)
+{
+ unsigned char enonce[TPM_NONCE_SIZE];
+ unsigned char ononce[TPM_NONCE_SIZE];
+ int ret;
+
+ ret = tpm_get_random(tb, ononce, TPM_NONCE_SIZE);
+ if (ret < 0)
+ return ret;
+
+ INIT_BUF(tb);
+ store16(tb, TPM_TAG_RQU_COMMAND);
+ store32(tb, TPM_OSAP_SIZE);
+ store32(tb, TPM_ORD_OSAP);
+ store16(tb, type);
+ store32(tb, handle);
+ storebytes(tb, ononce, TPM_NONCE_SIZE);
+
+ ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, TPM_MAX_BUF_SIZE);
+ if (ret < 0)
+ return ret;
+
+ s->handle = LOAD32(tb->data, TPM_DATA_OFFSET);
+ memcpy(s->enonce, &(tb->data[TPM_DATA_OFFSET + TPM_U32_SIZE]),
+ TPM_NONCE_SIZE);
+ memcpy(enonce, &(tb->data[TPM_DATA_OFFSET + TPM_U32_SIZE +
+ TPM_NONCE_SIZE]), TPM_NONCE_SIZE);
+ ret = TSS_rawhmac(s->secret, key, TPM_HASH_SIZE, TPM_NONCE_SIZE,
+ enonce, TPM_NONCE_SIZE, ononce, 0, 0);
+ return ret;
+}
+
+/*
+ * Create an object independent authorisation protocol (oiap) session
+ */
+static int oiap(struct tpm_buf *tb, uint32_t * handle, unsigned char *nonce)
+{
+ int ret;
+
+ INIT_BUF(tb);
+ store16(tb, TPM_TAG_RQU_COMMAND);
+ store32(tb, TPM_OIAP_SIZE);
+ store32(tb, TPM_ORD_OIAP);
+ ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, TPM_MAX_BUF_SIZE);
+ if (ret < 0)
+ return ret;
+
+ *handle = LOAD32(tb->data, TPM_DATA_OFFSET);
+ memcpy(nonce, &tb->data[TPM_DATA_OFFSET + TPM_U32_SIZE],
+ TPM_NONCE_SIZE);
+ return ret;
+}
+
+/*
+ * Have the TPM seal(encrypt) the trusted key, possibly based on
+ * Platform Configuration Registers (PCRs). AUTH1 for sealing key.
+ */
+static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
+ uint32_t keyhandle, unsigned char *keyauth,
+ unsigned char *data, uint32_t datalen,
+ unsigned char *blob, uint32_t * bloblen,
+ unsigned char *blobauth,
+ unsigned char *pcrinfo, uint32_t pcrinfosize)
+{
+ struct osapsess sess;
+ unsigned char encauth[TPM_HASH_SIZE];
+ unsigned char pubauth[TPM_HASH_SIZE];
+ unsigned char xorwork[TPM_HASH_SIZE * 2];
+ unsigned char xorhash[TPM_HASH_SIZE];
+ unsigned char nonceodd[TPM_NONCE_SIZE];
+ unsigned char cont;
+ uint32_t ordinal;
+ uint32_t pcrsize;
+ uint32_t datsize;
+ int sealinfosize;
+ int encdatasize;
+ int storedsize;
+ int ret;
+ int i;
+
+ ret = osap(tb, &sess, keyauth, keytype, keyhandle);
+ if (ret < 0)
+ return ret;
+ dump_sess(&sess);
+
+ /* calculate encrypted authorization value */
+ memcpy(xorwork, sess.secret, TPM_HASH_SIZE);
+ memcpy(xorwork + TPM_HASH_SIZE, sess.enonce, TPM_HASH_SIZE);
+ ret = TSS_sha1(xorwork, TPM_HASH_SIZE * 2, xorhash);
+ if (ret != 0)
+ return ret;
+
+ ret = tpm_get_random(tb, nonceodd, TPM_NONCE_SIZE);
+ if (ret < 0)
+ return ret;
+ ordinal = htonl(TPM_ORD_SEAL);
+ datsize = htonl(datalen);
+ pcrsize = htonl(pcrinfosize);
+ cont = 0;
+
+ /* encrypt data authorization key */
+ for (i = 0; i < TPM_HASH_SIZE; ++i)
+ encauth[i] = xorhash[i] ^ blobauth[i];
+
+ /* calculate authorization HMAC value */
+ if (pcrinfosize == 0) {
+ /* no pcr info specified */
+ TSS_authhmac(pubauth, sess.secret, TPM_HASH_SIZE,
+ sess.enonce, nonceodd, cont, TPM_U32_SIZE,
+ &ordinal, TPM_HASH_SIZE, encauth,
+ TPM_U32_SIZE, &pcrsize, TPM_U32_SIZE,
+ &datsize, datalen, data, 0, 0);
+ } else {
+ /* pcr info specified */
+ TSS_authhmac(pubauth, sess.secret, TPM_HASH_SIZE,
+ sess.enonce, nonceodd, cont, TPM_U32_SIZE,
+ &ordinal, TPM_HASH_SIZE, encauth,
+ TPM_U32_SIZE, &pcrsize, pcrinfosize,
+ pcrinfo, TPM_U32_SIZE, &datsize, datalen,
+ data, 0, 0);
+ }
+
+ INIT_BUF(tb);
+ store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
+ store32(tb, TPM_SEAL_SIZE + pcrinfosize + datalen);
+ store32(tb, TPM_ORD_SEAL);
+ store32(tb, keyhandle);
+ storebytes(tb, encauth, TPM_HASH_SIZE);
+ store32(tb, pcrinfosize);
+ storebytes(tb, pcrinfo, pcrinfosize);
+ store32(tb, datalen);
+ storebytes(tb, data, datalen);
+ store32(tb, sess.handle);
+ storebytes(tb, nonceodd, TPM_NONCE_SIZE);
+ store8(tb, cont);
+ storebytes(tb, pubauth, TPM_HASH_SIZE);
+
+ ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, TPM_MAX_BUF_SIZE);
+ if (ret < 0)
+ return ret;
+
+ /* calculate the size of the returned Blob */
+ sealinfosize = LOAD32(tb->data, TPM_DATA_OFFSET + TPM_U32_SIZE);
+ encdatasize = LOAD32(tb->data, TPM_DATA_OFFSET + TPM_U32_SIZE +
+ TPM_U32_SIZE + sealinfosize);
+ storedsize = TPM_U32_SIZE + TPM_U32_SIZE + sealinfosize +
+ TPM_U32_SIZE + encdatasize;
+
+ /* check the HMAC in the response */
+ ret = TSS_checkhmac1(tb->data, ordinal, nonceodd, sess.secret,
+ TPM_HASH_SIZE, storedsize, TPM_DATA_OFFSET, 0, 0);
+
+ /* copy the returned blob to caller */
+ memcpy(blob, tb->data + TPM_DATA_OFFSET, storedsize);
+ *bloblen = storedsize;
+ return ret;
+}
+
+/*
+ * use the AUTH2_COMMAND form of unseal, to authorize both key and blob
+ */
+static int tpm_unseal(struct tpm_buf *tb,
+ uint32_t keyhandle, unsigned char *keyauth,
+ unsigned char *blob, int bloblen,
+ unsigned char *blobauth,
+ unsigned char *data, unsigned int *datalen)
+{
+ unsigned char nonceodd[TPM_NONCE_SIZE];
+ unsigned char enonce1[TPM_NONCE_SIZE];
+ unsigned char enonce2[TPM_NONCE_SIZE];
+ unsigned char authdata1[TPM_HASH_SIZE];
+ unsigned char authdata2[TPM_HASH_SIZE];
+ uint32_t authhandle1 = 0;
+ uint32_t authhandle2 = 0;
+ unsigned char cont = 0;
+ uint32_t ordinal;
+ uint32_t keyhndl;
+ int ret;
+
+ ret = oiap(tb, &authhandle1, enonce1);
+ if (ret < 0) {
+ pr_info("trusted_key: oiap failed (%d)\n", ret);
+ return ret;
+ }
+ ret = oiap(tb, &authhandle2, enonce2);
+ if (ret < 0) {
+ pr_info("trusted_key: oiap failed (%d)\n", ret);
+ return ret;
+ }
+
+ ordinal = htonl(TPM_ORD_UNSEAL);
+ keyhndl = htonl(SRKHANDLE);
+ ret = tpm_get_random(tb, nonceodd, TPM_NONCE_SIZE);
+ if (ret < 0) {
+ pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
+ return ret;
+ }
+
+ TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
+ enonce1, nonceodd, cont, TPM_U32_SIZE,
+ &ordinal, bloblen, blob, 0, 0);
+ TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
+ enonce2, nonceodd, cont, TPM_U32_SIZE,
+ &ordinal, bloblen, blob, 0, 0);
+
+ INIT_BUF(tb);
+ store16(tb, TPM_TAG_RQU_AUTH2_COMMAND);
+ store32(tb, TPM_UNSEAL_SIZE + bloblen);
+ store32(tb, TPM_ORD_UNSEAL);
+ store32(tb, keyhandle);
+ storebytes(tb, blob, bloblen);
+ store32(tb, authhandle1);
+ storebytes(tb, nonceodd, TPM_NONCE_SIZE);
+ store8(tb, cont);
+ storebytes(tb, authdata1, TPM_HASH_SIZE);
+ store32(tb, authhandle2);
+ storebytes(tb, nonceodd, TPM_NONCE_SIZE);
+ store8(tb, cont);
+ storebytes(tb, authdata2, TPM_HASH_SIZE);
+
+ ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, TPM_MAX_BUF_SIZE);
+ if (ret < 0) {
+ pr_info("trusted_key: authhmac failed (%d)\n", ret);
+ return ret;
+ }
+
+ *datalen = LOAD32(tb->data, TPM_DATA_OFFSET);
+ ret = TSS_checkhmac2(tb->data, ordinal, nonceodd,
+ keyauth, TPM_HASH_SIZE,
+ blobauth, TPM_HASH_SIZE,
+ TPM_U32_SIZE, TPM_DATA_OFFSET,
+ *datalen, TPM_DATA_OFFSET + TPM_U32_SIZE, 0, 0);
+ if (ret < 0)
+ pr_info("trusted_key: TSS_checkhmac2 failed (%d)\n", ret);
+ memcpy(data, tb->data + TPM_DATA_OFFSET + TPM_U32_SIZE, *datalen);
+ return ret;
+}
+
+/*
+ * Have the TPM seal(encrypt) the symmetric key
+ */
+static int key_seal(struct trusted_key_payload *p,
+ struct trusted_key_options *o)
+{
+ struct tpm_buf *tb;
+ int ret;
+
+ tb = kzalloc(sizeof *tb, GFP_KERNEL);
+ if (!tb)
+ return -ENOMEM;
+
+ /* include migratable flag at end of sealed key */
+ p->key[p->key_len] = p->migratable;
+
+ ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
+ p->key, p->key_len + 1, p->blob, &p->blob_len,
+ o->blobauth, o->pcrinfo, o->pcrinfo_len);
+ if (ret < 0)
+ pr_info("trusted_key: srkseal failed (%d)\n", ret);
+
+ kfree(tb);
+ return ret;
+}
+
+/*
+ * Have the TPM unseal(decrypt) the symmetric key
+ */
+static int key_unseal(struct trusted_key_payload *p,
+ struct trusted_key_options *o)
+{
+ struct tpm_buf *tb;
+ int ret;
+
+ tb = kzalloc(sizeof *tb, GFP_KERNEL);
+ if (!tb)
+ return -ENOMEM;
+
+ ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
+ o->blobauth, p->key, &p->key_len);
+ /* pull migratable flag out of sealed key */
+ p->migratable = p->key[p->key_len--];
+
+ if (ret < 0)
+ pr_info("trusted_key: srkunseal failed (%d)\n", ret);
+
+ kfree(tb);
+ return ret;
+}
+
+enum {
+ Opt_err = -1,
+ Opt_new = 1, Opt_load, Opt_update,
+ Opt_keyhandle, Opt_keyauth, Opt_blobauth,
+ Opt_pcrinfo, Opt_pcrlock, Opt_migratable
+};
+
+static match_table_t key_tokens = {
+ {Opt_new, "new"},
+ {Opt_load, "load"},
+ {Opt_update, "update"},
+ {Opt_keyhandle, "keyhandle=%s"},
+ {Opt_keyauth, "keyauth=%s"},
+ {Opt_blobauth, "blobauth=%s"},
+ {Opt_pcrinfo, "pcrinfo=%s"},
+ {Opt_pcrlock, "pcrlock=%s"},
+ {Opt_migratable, "migratable=%s"},
+ {Opt_err, NULL}
+};
+
+/* can have zero or more token= options */
+static int getoptions(char *c, struct trusted_key_payload *pay,
+ struct trusted_key_options *opt)
+{
+ substring_t args[MAX_OPT_ARGS];
+ char *p = c;
+ int token;
+ int res;
+ unsigned long handle;
+ unsigned long lock;
+
+ while ((p = strsep(&c, " \t"))) {
+ if ((*p == '\0') || (*p == ' ') || (*p == '\t'))
+ continue;
+ token = match_token(p, key_tokens, args);
+
+ switch (token) {
+ case Opt_pcrinfo:
+ opt->pcrinfo_len = strlen(args[0].from) / 2;
+ if (opt->pcrinfo_len > MAX_PCRINFO_SIZE)
+ return -EINVAL;
+ hex2bin(opt->pcrinfo, args[0].from, opt->pcrinfo_len);
+ break;
+ case Opt_keyhandle:
+ res = strict_strtoul(args[0].from, 16, &handle);
+ if (res < 0)
+ return -EINVAL;
+ opt->keytype = SEALKEYTYPE;
+ opt->keyhandle = (uint32_t) handle;
+ break;
+ case Opt_keyauth:
+ if (strlen(args[0].from) != 2 * TPM_HASH_SIZE)
+ return -EINVAL;
+ hex2bin(opt->keyauth, args[0].from, TPM_HASH_SIZE);
+ break;
+ case Opt_blobauth:
+ if (strlen(args[0].from) != 2 * TPM_HASH_SIZE)
+ return -EINVAL;
+ hex2bin(opt->blobauth, args[0].from, TPM_HASH_SIZE);
+ break;
+ case Opt_migratable:
+ if (*args[0].from == '0')
+ pay->migratable = 0;
+ else
+ return -EINVAL;
+ break;
+ case Opt_pcrlock:
+ res = strict_strtoul(args[0].from, 10, &lock);
+ if (res < 0)
+ return -EINVAL;
+ opt->pcrlock = (int)lock;
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
+
+/*
+ * datablob_parse - parse the keyctl data and fill in the
+ * payload and options structures
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int datablob_parse(char *datablob, struct trusted_key_payload *p,
+ struct trusted_key_options *o)
+{
+ substring_t args[MAX_OPT_ARGS];
+ int ret = -EINVAL;
+ int key_cmd;
+ char *c;
+
+ /* main command */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ key_cmd = match_token(c, key_tokens, args);
+ switch (key_cmd) {
+ case Opt_new:
+ /* first argument is key size */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ ret = strict_strtol(c, 10, (long *)&p->key_len);
+ if ((ret < 0) || (p->key_len < MIN_KEY_SIZE) ||
+ (p->key_len > MAX_KEY_SIZE))
+ return -EINVAL;
+ ret = getoptions(datablob, p, o);
+ if (ret)
+ return -EINVAL;
+ ret = Opt_new;
+ break;
+ case Opt_load:
+ /* first argument is sealed blob */
+ c = strsep(&datablob, " \t");
+ if (!c)
+ return -EINVAL;
+ p->blob_len = strlen(c) / 2;
+ if (p->blob_len > MAX_BLOB_SIZE)
+ return -EINVAL;
+ hex2bin(p->blob, c, p->blob_len);
+ ret = getoptions(datablob, p, o);
+ if (ret)
+ return -EINVAL;
+ ret = Opt_load;
+ break;
+ case Opt_update:
+ /* all arguments are options */
+ ret = getoptions(datablob, p, o);
+ if (ret)
+ return -EINVAL;
+ ret = Opt_update;
+ break;
+ case Opt_err:
+ return -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static struct trusted_key_options *trusted_options_alloc(void)
+{
+ struct trusted_key_options *options;
+
+ options = kzalloc(sizeof *options, GFP_KERNEL);
+ if (!options)
+ return options;
+
+ /* set any non-zero defaults */
+ options->keytype = SRKKEYTYPE;
+ options->keyhandle = SRKHANDLE;
+ return options;
+}
+
+static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
+{
+ struct trusted_key_payload *p = NULL;
+ int ret;
+
+ ret = key_payload_reserve(key, sizeof *p);
+ if (ret < 0)
+ return p;
+ p = kzalloc(sizeof *p, GFP_KERNEL);
+
+ /* migratable by default */
+ p->migratable = 1;
+ return p;
+}
+
+/*
+ * trusted_instantiate - create a new trusted key
+ *
+ * Unseal an existing trusted blob or, for a new key, get a
+ * random key, then seal and create a trusted key-type key,
+ * adding it to the specified keyring.
+ *
+ * On success, return 0. Otherwise return errno.
+ */
+static int trusted_instantiate(struct key *key, const void *data,
+ size_t datalen)
+{
+ struct trusted_key_payload *payload = NULL;
+ struct trusted_key_options *options = NULL;
+ char *datablob;
+ int ret = 0;
+ int key_cmd;
+
+ if (datalen <= 0 || datalen > 32767 || !data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+ memcpy(datablob, data, datalen);
+ *(datablob + datalen) = '\0';
+
+ options = trusted_options_alloc();
+ if (!options) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ payload = trusted_payload_alloc(key);
+ if (!payload) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ key_cmd = datablob_parse(datablob, payload, options);
+ if (key_cmd < 0) {
+ ret = key_cmd;
+ goto out;
+ }
+
+ dump_payload(payload);
+ dump_options(options);
+
+ switch (key_cmd) {
+ case Opt_load:
+ ret = key_unseal(payload, options);
+ if (ret < 0)
+ pr_info("trusted_key: key_unseal failed (%d)\n", ret);
+ break;
+ case Opt_new:
+ ret = my_get_random(payload->key, payload->key_len);
+ if (ret < 0) {
+ pr_info("trusted_key: key_create failed (%d)\n", ret);
+ goto out;
+ }
+ ret = key_seal(payload, options);
+ if (ret < 0)
+ pr_info("trusted_key: key_seal failed (%d)\n", ret);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ if (options->pcrlock)
+ ret = pcrlock(options->pcrlock);
+out:
+ kfree(datablob);
+ if (options)
+ kfree(options);
+ if (!ret)
+ rcu_assign_pointer(key->payload.data, payload);
+ else if (payload)
+ kfree(payload);
+ return ret;
+}
+
+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);
+}
+
+/*
+ * trusted_update - reseal an existing key with new PCR values
+ */
+static int trusted_update(struct key *key, const void *data, size_t datalen)
+{
+ struct trusted_key_payload *p = key->payload.data;
+ struct trusted_key_payload *new_p;
+ struct trusted_key_options *new_o;
+ char *datablob;
+ int ret = 0;
+
+ if (!p->migratable)
+ return -EPERM;
+
+ if (datalen <= 0 || datalen > 32767 || !data)
+ return -EINVAL;
+
+ datablob = kmalloc(datalen + 1, GFP_KERNEL);
+ if (!datablob)
+ return -ENOMEM;
+ new_o = trusted_options_alloc();
+ if (!new_o) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ new_p = trusted_payload_alloc(key);
+ if (!new_p) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ memcpy(datablob, data, datalen);
+ *(datablob + datalen) = '\0';
+ ret = datablob_parse(datablob, new_p, new_o);
+ if (ret != Opt_update) {
+ ret = -EINVAL;
+ goto out;
+ }
+ /* copy old key values, and reseal with new pcrs */
+ new_p->migratable = p->migratable;
+ new_p->key_len = p->key_len;
+ memcpy(new_p->key, p->key, p->key_len);
+ dump_payload(p);
+ dump_payload(new_p);
+
+ ret = key_seal(new_p, new_o);
+ if (ret < 0) {
+ pr_info("trusted_key: key_seal failed (%d)\n", ret);
+ kfree(new_p);
+ goto out;
+ }
+ if (new_o->pcrlock)
+ ret = pcrlock(new_o->pcrlock);
+ rcu_assign_pointer(key->payload.data, new_p);
+ call_rcu(&p->rcu, trusted_rcu_free);
+out:
+ kfree(datablob);
+ if (new_o)
+ kfree(new_o);
+ return ret;
+}
+
+/*
+ * trusted_read - copy the sealed blob data to userspace in hex.
+ * On success, return to userspace the trusted key datablob size.
+ */
+static long trusted_read(const struct key *key, char __user * buffer,
+ size_t buflen)
+{
+ struct trusted_key_payload *p;
+ char *ascii_buf;
+ char *bufp;
+ int i;
+
+ p = rcu_dereference_protected(key->payload.data,
+ rwsem_is_locked(&((struct key *)key)->
+ sem));
+ 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 = pack_hex_byte(bufp, p->blob[i]);
+
+ if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) {
+ kfree(ascii_buf);
+ return -EFAULT;
+ }
+ kfree(ascii_buf);
+ return 2 * p->blob_len;
+}
+
+/*
+ * trusted_destroy - before freeing the key, clear the decrypted data
+ */
+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);
+}
+
+struct key_type key_type_trusted = {
+ .name = "trusted",
+ .instantiate = trusted_instantiate,
+ .update = trusted_update,
+ .match = user_match,
+ .destroy = trusted_destroy,
+ .describe = user_describe,
+ .read = trusted_read,
+};
+
+EXPORT_SYMBOL_GPL(key_type_trusted);
+
+static int __init init_trusted(void)
+{
+ int ret;
+
+ ret = register_key_type(&key_type_trusted);
+ if (ret < 0)
+ return ret;
+ return ret;
+}
+
+static void __exit cleanup_trusted(void)
+{
+ unregister_key_type(&key_type_trusted);
+}
+
+module_init(init_trusted);
+module_exit(cleanup_trusted);
+
+MODULE_LICENSE("GPL");
diff --git a/security/keys/trusted_defined.h b/security/keys/trusted_defined.h
new file mode 100644
index 0000000..113e6b7
--- /dev/null
+++ b/security/keys/trusted_defined.h
@@ -0,0 +1,147 @@
+#ifndef __TRUSTED_KEY_H
+#define __TRUSTED_KEY_H
+
+#define TPM_MAX_BUF_SIZE 512
+#define TPM_TAG_RQU_COMMAND 193
+#define TPM_TAG_RQU_AUTH1_COMMAND 194
+#define TPM_TAG_RQU_AUTH2_COMMAND 195
+#define TPM_TAG_RSP_COMMAND 196
+#define TPM_TAG_RSP_AUTH1_COMMAND 197
+#define TPM_TAG_RSP_AUTH2_COMMAND 198
+#define TPM_NONCE_SIZE 20
+#define TPM_HASH_SIZE 20
+#define TPM_SIZE_OFFSET 2
+#define TPM_RETURN_OFFSET 6
+#define TPM_DATA_OFFSET 10
+#define TPM_U32_SIZE 4
+#define TPM_GETRANDOM_SIZE 14
+#define TPM_GETRANDOM_RETURN 14
+#define TPM_ORD_GETRANDOM 70
+#define TPM_RESET_SIZE 10
+#define TPM_ORD_RESET 90
+#define TPM_OSAP_SIZE 36
+#define TPM_ORD_OSAP 11
+#define TPM_OIAP_SIZE 10
+#define TPM_ORD_OIAP 10
+#define TPM_SEAL_SIZE 87
+#define TPM_ORD_SEAL 23
+#define TPM_ORD_UNSEAL 24
+#define TPM_UNSEAL_SIZE 104
+#define SEALKEYTYPE 1
+#define SRKKEYTYPE 4
+#define SRKHANDLE 0x40000000
+#define TPM_ANY_NUM 0xFFFF
+#define MAX_PCRINFO_SIZE 64
+
+#define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
+#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
+#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
+
+struct tpm_buf {
+ int len;
+ unsigned char data[TPM_MAX_BUF_SIZE];
+};
+
+#define INIT_BUF(tb) (tb->len = 0)
+
+struct osapsess {
+ uint32_t handle;
+ unsigned char secret[TPM_HASH_SIZE];
+ unsigned char enonce[TPM_NONCE_SIZE];
+};
+
+struct trusted_key_options {
+ uint16_t keytype;
+ uint32_t keyhandle;
+ unsigned char keyauth[TPM_HASH_SIZE];
+ unsigned char blobauth[TPM_HASH_SIZE];
+ uint32_t pcrinfo_len;
+ unsigned char pcrinfo[MAX_PCRINFO_SIZE];
+ int pcrlock;
+};
+
+#define TPM_DEBUG 0
+
+#if TPM_DEBUG
+static inline void dump_options(struct trusted_key_options *o)
+{
+ pr_info("trusted_key: sealing key type %d\n", o->keytype);
+ pr_info("trusted_key: sealing key handle %0X\n", o->keyhandle);
+ pr_info("trusted_key: pcrlock %d\n", o->pcrlock);
+ pr_info("trusted_key: pcrinfo %d\n", o->pcrinfo_len);
+ print_hex_dump(KERN_INFO, "pcrinfo ", DUMP_PREFIX_NONE,
+ 16, 1, o->pcrinfo, o->pcrinfo_len, 0);
+}
+
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+ pr_info("trusted_key: key_len %d\n", p->key_len);
+ print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
+ 16, 1, p->key, p->key_len, 0);
+ pr_info("trusted_key: bloblen %d\n", p->blob_len);
+ print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
+ 16, 1, p->blob, p->blob_len, 0);
+ pr_info("trusted_key: migratable %d\n", p->migratable);
+}
+
+static inline void dump_sess(struct osapsess *s)
+{
+ print_hex_dump(KERN_INFO, "trusted-key: handle ", DUMP_PREFIX_NONE,
+ 16, 1, &s->handle, 4, 0);
+ pr_info("trusted-key: secret:\n");
+ print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE,
+ 16, 1, &s->secret, TPM_HASH_SIZE, 0);
+ pr_info("trusted-key: enonce:\n");
+ print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE,
+ 16, 1, &s->enonce, TPM_HASH_SIZE, 0);
+}
+
+static inline void dump_tpm_buf(unsigned char *buf)
+{
+ int len;
+
+ pr_info("\ntrusted-key: tpm buffer\n");
+ len = LOAD32(buf, TPM_SIZE_OFFSET);
+ print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, 16, 1, buf, len, 0);
+}
+#else
+static inline void dump_options(struct trusted_key_options *o)
+{
+}
+
+static inline void dump_payload(struct trusted_key_payload *p)
+{
+}
+
+static inline void dump_sess(struct osapsess *s)
+{
+}
+
+static inline void dump_tpm_buf(unsigned char *buf)
+{
+}
+#endif
+
+static inline void store8(struct tpm_buf *buf, unsigned char value)
+{
+ buf->data[buf->len++] = value;
+}
+
+static inline void store16(struct tpm_buf *buf, uint16_t value)
+{
+ *(uint16_t *) & buf->data[buf->len] = htons(value);
+ buf->len += sizeof(value);
+}
+
+static inline void store32(struct tpm_buf *buf, uint32_t value)
+{
+ *(uint32_t *) & buf->data[buf->len] = htonl(value);
+ buf->len += sizeof(value);
+}
+
+static inline void storebytes(struct tpm_buf *buf, unsigned char *in, int len)
+{
+ memcpy(buf->data + buf->len, in, len);
+ buf->len += len;
+}
+#endif
--
1.7.2.2

2010-11-10 15:52:20

[permalink] [raw]

Subject: [PATCH v1.3 1/4] lib: hex2bin converts ascii hexadecimal string to binary

Similar to the kgdb_hex2mem() code, hex2bin converts a string
to binary using the hex_to_bin() library call.

Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: Serge E. Hallyn <[email protected]>
---
include/linux/kernel.h | 1 +
lib/hexdump.c | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3f648d2..b03aa43 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -419,6 +419,7 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
}

extern int hex_to_bin(char ch);
+extern void hex2bin(unsigned char *mem, char *buf, int count);

#ifndef pr_fmt
#define pr_fmt(fmt) fmt
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 5d7a480..66f96bb 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -34,6 +34,22 @@ int hex_to_bin(char ch)
EXPORT_SYMBOL(hex_to_bin);

/**
+ * hex2bin - convert an ascii hexadecimal string to its binary representation
+ * @mem: result
+ * @buf: ascii hexadecimal string
+ * @count: result length
+ */
+void hex2bin(unsigned char *mem, char *buf, int count)
+{
+ while (count--) {
+ *mem = hex_to_bin(*buf++) << 4;
+ *mem += hex_to_bin(*buf++);
+ mem++;
+ }
+}
+EXPORT_SYMBOL(hex2bin);
+
+/**
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
* @len: number of bytes in the @buf
--
1.7.2.2

2010-11-11 19:49:22

[permalink] [raw]

Subject: Re: [PATCH v1.3 1/4] lib: hex2bin converts ascii hexadecimal string to binary

Mimi Zohar <[email protected]> wrote:

> +void hex2bin(unsigned char *mem, char *buf, int count)

I think this needs a little adjustment. I would recommend something like the
following declaration:

void hex2bin(u8 *buf, const char *data, size_t count)

since the output data is binary (so use u8*), you're only reading the source
data (so should use a const pointer) and you don't want to see a negative
count (so use size_t).

Also, I'd suggest calling the source arg 'data' and the output arg 'buf' or
maybe src/dst.

David

2010-11-11 19:49:06

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

Mimi Zohar <[email protected]> wrote:

> Reviewd-by: Mimi Zohar <[email protected]>

You've missed an 'e'.

> +int tpm_send(u32 chip_num, char *cmd, int buflen)
> +{
> ...
> + rc = transmit_cmd(chip, (struct tpm_cmd_t *)cmd, buflen,
> + "attempting tpm_cmd");

Make cmd 'void *' to obviate the cast. Preferably it should be const too.

> + module_put(chip->dev->driver->owner);

Where's the corresponding module_get()? I suspect this should be wrapped to
match tpm_chip_find_get().

David

2010-11-11 21:58:19

[permalink] [raw]

Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type

Mimi Zohar <[email protected]> wrote:

> Defines a new kernel key-type called 'trusted'. Trusted keys are
> random number symmetric keys, generated and RSA-sealed by the TPM.
> The TPM only unseals the keys, if the boot PCRs and other criteria
> match. Userspace can only ever see encrypted blobs.

I'd recommend saying 'Define' not 'Defines'. You're talking in first or
second person.

I'd also recommend filling out the text to 80 chars and being consistent about
the use of space after a full stop (you sometimes use 1 and sometimes 2 spaces
- I'd recommend 2).

> include/keys/trusted-type.h | 32 ++
> security/Kconfig | 15 +
> security/keys/Makefile | 1 +
> security/keys/trusted_defined.c | 1101 +++++++++++++++++++++++++++++++++++++++
> security/keys/trusted_defined.h | 147 ++++++
> 5 files changed, 1296 insertions(+), 0 deletions(-)
> create mode 100644 include/keys/trusted-type.h
> create mode 100644 security/keys/trusted_defined.c
> create mode 100644 security/keys/trusted_defined.h

Documentation/trusted_keys.txt?

> @@ -0,0 +1,32 @@
> +/* trusted-type.h: trusted-defined key type

There's no need to include the name of the file here. We know what the name
of the file is.

> +struct trusted_key_payload {
> + struct rcu_head rcu; /* RCU destructor */

The comment is superfluous. That's what that struct is for.

> + unsigned char key[MAX_KEY_SIZE+1];

In general, there should be spaces either side of binary operators like '+'.

> +config TRUSTED_KEYS
> + tristate "TRUSTED KEYS"
> + depends on KEYS && TCG_TPM
> + select CRYPTO
> + select CRYPTO_HMAC
> + select CRYPTO_SHA1
> + help
> + This option provides support for creating, sealing, and unsealing
> + keys in the kernel. Trusted keys are random number symmetric keys,
> + generated and RSA-sealed by the TPM. The TPM only unseals the keys,

Superfluous comma.

> + if the boot PCRs and other criteria match. Userspace can only ever
> + see encrypted blobs.

I'd say 'will' rather than 'can'.

> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> new file mode 100644
> index 0000000..d45964d
> --- /dev/null
> +++ b/security/keys/trusted_defined.c
> @@ -0,0 +1,1101 @@
> +/*
> + * Copyright (C) 2010 IBM Corporation
> + *
> + * Author:
> + * David Safford <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + *
> + * Defines a new kernel key-type called 'trusted'. Trusted keys are random

Again 'Define'.

> + * number symmetric keys, generated and RSA-sealed by the TPM. The TPM only
> + * unseals the keys, if the boot PCRs and other criteria match. Userspace
> + * can only ever see encrypted blobs.

And two spaces after full stops please.

> + * By default, trusted keys are sealed with the Storage Root Key (SRK).
> + * The SRK usage authorization value is the well-known value of 20 zeroes.
> + * This can be set at takeownership time with the trouser's utility
> + * "tpm_takeownership -u -z".
> + *
> + * Usage:
> + * keyctl add trusted name "new keylen [options]" ring
> + * keyctl update key "update [options]"
> + * keyctl add trusted name "load hex_blob [pcrlock=pcrnum]" ring
> + * keyctl print keyid
> + *
> + * options:
> + * keyhandle= ascii hex value of sealing key
> + * default 0x40000000 (SRK)
> + * keyauth= ascii hex auth for sealing key
> + * default 0x00... (40 ascii zeros)
> + * blobauth= ascii hex auth for sealed data
> + * default 0x00... (40 ascii zeros)
> + * pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG
> + * (no default - create with genpcrinfo util)
> + * pcrlock= pcr number to be extended to "lock" blob
> + * migratable= 0|1 indicating permission to reseal to new PCR values
> + * default 1 (resealing allowed)
> + *
> + * keyctl print returns an ascii hex copy of the sealed key, which is in
> + * standard TPM_STORED_DATA format.
> + * keys can be 32 - 128 bytes, blob max is 1024 hex ascii characters
> + * binary pcrinfo max is 512 hex ascii characters

I would put this information in a file in the Documentation directory and put
a pointer to the file here. You can then expand on the command structure
you've defined in the documentation.

> +static char hmac_alg[] = "hmac(sha1)";
> +static char hash_alg[] = "sha1";

Should be const.

> +static int init_sha1_desc(struct hash_desc *desc)
> +{
> + int rc;
> +
> + desc->tfm = crypto_alloc_hash(hash_alg, 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(desc->tfm)) {
> + rc = PTR_ERR(desc->tfm);
> + return rc;

Those should be merged into one line which would allow you to ditch the braces
also.

> +static int init_hmac_desc(struct hash_desc *desc, unsigned char *key,
> + int keylen)
> +{
> + int rc;
> +
> + desc->tfm = crypto_alloc_hash(hmac_alg, 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(desc->tfm)) {
> + rc = PTR_ERR(desc->tfm);
> + return rc;

Merge.

> +static int TSS_sha1(unsigned char *data, int datalen, unsigned char *digest)
> +{
> + struct hash_desc desc;
> + struct scatterlist sg[1];
> + int rc;
> +
> + rc = init_sha1_desc(&desc);
> + if (rc != 0)
> + return rc;

I'd recommend you be consistent about the use of (rc != 0) and (rc < 0) to
check for errors.

> + sg_init_one(sg, data, datalen);

You should use the shash interface, not the hash interface. That way you can
avoid the scatter-gather lists.

> + rc = crypto_hash_update(&desc, sg, datalen);
> + if (rc < 0)
> + goto out;
> + rc = crypto_hash_final(&desc, digest);
> +out:

You could dispense with the goto and the label simply by inverting the logic
of the if-condition.

> +static int TSS_rawhmac(unsigned char *digest, unsigned char *key,
> + unsigned int keylen, ...)
> +{
> + struct hash_desc desc;
> + struct scatterlist sg[1];

Again, use shash rather than hash to avoid the SG interface.

> + unsigned int dlen;
> + unsigned char *data;
> + va_list argp;
> + int error;
> +
> + error = init_hmac_desc(&desc, key, keylen);
> + if (error)
> + return error;
> +
> + va_start(argp, keylen);
> + for (;;) {
> + dlen = (unsigned int)va_arg(argp, unsigned int);

Doesn't va_arg() cast for you?

> + if (dlen == 0)
> + break;
> + data = (unsigned char *)va_arg(argp, unsigned char *);

Ditto, plus several more within the file.

> +/*
> + * Have the TPM seal(encrypt) the trusted key, possibly based on
> + * Platform Configuration Registers (PCRs). AUTH1 for sealing key.
> + */
> +static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> + uint32_t keyhandle, unsigned char *keyauth,
> + unsigned char *data, uint32_t datalen,
> + unsigned char *blob, uint32_t * bloblen,
> + unsigned char *blobauth,
> + unsigned char *pcrinfo, uint32_t pcrinfosize)
> +{
> + struct osapsess sess;
> + unsigned char encauth[TPM_HASH_SIZE];
> + unsigned char pubauth[TPM_HASH_SIZE];
> + unsigned char xorwork[TPM_HASH_SIZE * 2];
> + unsigned char xorhash[TPM_HASH_SIZE];
> + unsigned char nonceodd[TPM_NONCE_SIZE];

That's quite a lot of stack space, and you're calling other functions that
also allocate chunks of stack space.

David

2010-11-11 22:23:26

[permalink] [raw]

Subject: Re: [PATCH v1.3 1/4] lib: hex2bin converts ascii hexadecimal string to binary

On Thu, 2010-11-11 at 19:48 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > +void hex2bin(unsigned char *mem, char *buf, int count)
>
> I think this needs a little adjustment. I would recommend something like the
> following declaration:
>
> void hex2bin(u8 *buf, const char *data, size_t count)
>
> since the output data is binary (so use u8*), you're only reading the source
> data (so should use a const pointer) and you don't want to see a negative
> count (so use size_t).

right, thanks.

> Also, I'd suggest calling the source arg 'data' and the output arg 'buf' or
> maybe src/dst.
>
> David

Definitely better. (Remnant from kgdb_hex2mem() naming.)

thanks,

Mimi

2010-11-11 22:26:08

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

On Thu, 2010-11-11 at 19:48 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > Reviewd-by: Mimi Zohar <[email protected]>
>
> You've missed an 'e'.

thanks, will fix

> > +int tpm_send(u32 chip_num, char *cmd, int buflen)
> > +{
> > ...
> > + rc = transmit_cmd(chip, (struct tpm_cmd_t *)cmd, buflen,
> > + "attempting tpm_cmd");
>
> Make cmd 'void *' to obviate the cast. Preferably it should be const too.

will do

> > + module_put(chip->dev->driver->owner);
>
> Where's the corresponding module_get()? I suspect this should be wrapped to
> match tpm_chip_find_get().
>
> David

The module_get() is in tpm_chip_find_get(), which is just a helper.
(It's used this way throughout tpm.c)

thanks,

Mimi

2010-11-12 12:59:59

[permalink] [raw]

Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type

On Thu, 2010-11-11 at 21:57 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:

Thanks for the helpful comments - much appreciated.
Willdo on all of them - just one question on the last comment:

> > +/*
> > + * Have the TPM seal(encrypt) the trusted key, possibly based on
> > + * Platform Configuration Registers (PCRs). AUTH1 for sealing key.
> > + */
> > +static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > + uint32_t keyhandle, unsigned char *keyauth,
> > + unsigned char *data, uint32_t datalen,
> > + unsigned char *blob, uint32_t * bloblen,
> > + unsigned char *blobauth,
> > + unsigned char *pcrinfo, uint32_t pcrinfosize)
> > +{
> > + struct osapsess sess;
> > + unsigned char encauth[TPM_HASH_SIZE];
> > + unsigned char pubauth[TPM_HASH_SIZE];
> > + unsigned char xorwork[TPM_HASH_SIZE * 2];
> > + unsigned char xorhash[TPM_HASH_SIZE];
> > + unsigned char nonceodd[TPM_NONCE_SIZE];
>
> That's quite a lot of stack space, and you're calling other functions that
> also allocate chunks of stack space.

checkstack showed that the max stack usage was for tpm_seal, at 530
bytes. (The rest were 300 or less.) I can certainly throw the hashes
and nonce (120 bytes) into a dynamically allocated struct, if you
think it is worth the extra code...

thanks
dave

2010-11-12 14:11:39

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

Mimi Zohar <[email protected]> wrote:

> > > + module_put(chip->dev->driver->owner);
> >
> > Where's the corresponding module_get()? I suspect this should be wrapped to
> > match tpm_chip_find_get().
> >
> > David
>
> The module_get() is in tpm_chip_find_get(), which is just a helper.
> (It's used this way throughout tpm.c)

I'd make a function tpm_chip_find_put() just to wrap module_put() and then
place it with tpm_chip_find_get() in the sources. That makes it easier to see
what's going on.

David

2010-11-12 14:49:20

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

On Fri, 2010-11-12 at 14:11 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > > > + module_put(chip->dev->driver->owner);
> > >
> > > Where's the corresponding module_get()? I suspect this should be wrapped to
> > > match tpm_chip_find_get().
> > >
> > > David
> >
> > The module_get() is in tpm_chip_find_get(), which is just a helper.
> > (It's used this way throughout tpm.c)
>
> I'd make a function tpm_chip_find_put() just to wrap module_put() and then
> place it with tpm_chip_find_get() in the sources. That makes it easier to see
> what's going on.

Or alternately rename the helper to tpm_chip_find_and_module_get()?
In either case, this is really up to Rajiv (the maintainer for tpm.c
who has already acked this patch), as this usage already appears in
other places in tpm.c. There would have to be a separate patch fixing
this for all of the instances. Rajiv, your thoughts?

dave

2010-11-12 16:53:45

[permalink] [raw]

Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type

Mimi Zohar <[email protected]> wrote:

> +enum {
> + Opt_err = -1,
> + Opt_new = 1, Opt_load, Opt_update,
> + Opt_keyhandle, Opt_keyauth, Opt_blobauth,
> + Opt_pcrinfo, Opt_pcrlock, Opt_migratable
> +};

The compiler can generate slightly more efficient code if you don't skip 0 in
your enum.

> +static match_table_t key_tokens = {

const.

> +static int getoptions(char *c, struct trusted_key_payload *pay,
> + struct trusted_key_options *opt)
> +{
> + substring_t args[MAX_OPT_ARGS];
> + char *p = c;
> + int token;
> + int res;
> + unsigned long handle;
> + unsigned long lock;
> +
> + while ((p = strsep(&c, " \t"))) {
> + if ((*p == '\0') || (*p == ' ') || (*p == '\t'))

Superfluous brackets round the individual comparisons.

> + continue;
> + token = match_token(p, key_tokens, args);
> +
> + switch (token) {
> + case Opt_pcrinfo:
> + opt->pcrinfo_len = strlen(args[0].from) / 2;
> + if (opt->pcrinfo_len > MAX_PCRINFO_SIZE)
> + return -EINVAL;
> + hex2bin(opt->pcrinfo, args[0].from, opt->pcrinfo_len);
> + break;
> + case Opt_keyhandle:
> + res = strict_strtoul(args[0].from, 16, &handle);
> + if (res < 0)
> + return -EINVAL;
> + opt->keytype = SEALKEYTYPE;
> + opt->keyhandle = (uint32_t) handle;

Unnecessary cast.

> + break;
> + case Opt_keyauth:
> + if (strlen(args[0].from) != 2 * TPM_HASH_SIZE)
> + return -EINVAL;
> + hex2bin(opt->keyauth, args[0].from, TPM_HASH_SIZE);
> + break;
> + case Opt_blobauth:
> + if (strlen(args[0].from) != 2 * TPM_HASH_SIZE)
> + return -EINVAL;
> + hex2bin(opt->blobauth, args[0].from, TPM_HASH_SIZE);
> + break;
> + case Opt_migratable:
> + if (*args[0].from == '0')
> + pay->migratable = 0;
> + else
> + return -EINVAL;
> + break;
> + case Opt_pcrlock:
> + res = strict_strtoul(args[0].from, 10, &lock);
> + if (res < 0)
> + return -EINVAL;
> + opt->pcrlock = (int)lock;

Unnecessary cast.

> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> + return 0;
> +}
> +
> +/*
> + * datablob_parse - parse the keyctl data and fill in the
> + * payload and options structures
> + *
> + * On success returns 0, otherwise -EINVAL.
> + */
> +static int datablob_parse(char *datablob, struct trusted_key_payload *p,
> + struct trusted_key_options *o)
> +{
> ...
> + ret = strict_strtol(c, 10, (long *)&p->key_len);

NAK! You cannot do this. It won't work on 64-bit machines, especially
big-endian ones. Casting the pointer does not change the size of the
destination variable. You must use a temporary var.

> + if ((ret < 0) || (p->key_len < MIN_KEY_SIZE) ||
> + (p->key_len > MAX_KEY_SIZE))

Superfluous parenthesization.

> +static int trusted_instantiate(struct key *key, const void *data,
> + size_t datalen)
> +{
> ...
> + switch (key_cmd) {
> + case Opt_load:
> + ret = key_unseal(payload, options);
> + if (ret < 0)
> + pr_info("trusted_key: key_unseal failed (%d)\n", ret);
> + break;
> + case Opt_new:
> + ret = my_get_random(payload->key, payload->key_len);
> + if (ret < 0) {
> + pr_info("trusted_key: key_create failed (%d)\n", ret);
> + goto out;
> + }
> + ret = key_seal(payload, options);
> + if (ret < 0)
> + pr_info("trusted_key: key_seal failed (%d)\n", ret);
> + break;

Aha! I see how this works now. Using add/update key seems the right way to
do things.

> + default:
> + ret = -EINVAL;
> + }
> + if (options->pcrlock)
> + ret = pcrlock(options->pcrlock);

Do you really want to go through pcrlock() if you're going to return -EINVAL?

> +out:
> + kfree(datablob);
> + if (options)
> + kfree(options);

kfree() can handle NULL pointers.

> + if (!ret)
> + rcu_assign_pointer(key->payload.data, payload);
> + else if (payload)
> + kfree(payload);

Again, kfree() can handle a NULL pointer.

> +#define TPM_MAX_BUF_SIZE 512
> +#define TPM_TAG_RQU_COMMAND 193
> +#define TPM_TAG_RQU_AUTH1_COMMAND 194
> +#define TPM_TAG_RQU_AUTH2_COMMAND 195
> +#define TPM_TAG_RSP_COMMAND 196
> +#define TPM_TAG_RSP_AUTH1_COMMAND 197
> +#define TPM_TAG_RSP_AUTH2_COMMAND 198
> +#define TPM_NONCE_SIZE 20
> +#define TPM_HASH_SIZE 20
> +#define TPM_SIZE_OFFSET 2
> +#define TPM_RETURN_OFFSET 6
> +#define TPM_DATA_OFFSET 10
> +#define TPM_U32_SIZE 4
> +#define TPM_GETRANDOM_SIZE 14
> +#define TPM_GETRANDOM_RETURN 14
> +#define TPM_ORD_GETRANDOM 70
> +#define TPM_RESET_SIZE 10
> +#define TPM_ORD_RESET 90
> +#define TPM_OSAP_SIZE 36
> +#define TPM_ORD_OSAP 11
> +#define TPM_OIAP_SIZE 10
> +#define TPM_ORD_OIAP 10
> +#define TPM_SEAL_SIZE 87
> +#define TPM_ORD_SEAL 23
> +#define TPM_ORD_UNSEAL 24
> +#define TPM_UNSEAL_SIZE 104
> +#define SEALKEYTYPE 1
> +#define SRKKEYTYPE 4
> +#define SRKHANDLE 0x40000000
> +#define TPM_ANY_NUM 0xFFFF
> +#define MAX_PCRINFO_SIZE 64

I suspect some of these should be in somewhere like linux/tpm.h rather than
here. They're specific to TPM access not TPM key management.

> +#define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> +#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
> +#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> +
> +struct tpm_buf {
> + int len;
> + unsigned char data[TPM_MAX_BUF_SIZE];
> +};
> +
> +#define INIT_BUF(tb) (tb->len = 0)
> +
> +struct osapsess {
> + uint32_t handle;
> + unsigned char secret[TPM_HASH_SIZE];
> + unsigned char enonce[TPM_NONCE_SIZE];
> +};
> +
> +struct trusted_key_options {
> + uint16_t keytype;

key type enum?

> + uint32_t keyhandle;
> + unsigned char keyauth[TPM_HASH_SIZE];
> + unsigned char blobauth[TPM_HASH_SIZE];
> + uint32_t pcrinfo_len;
> + unsigned char pcrinfo[MAX_PCRINFO_SIZE];
> + int pcrlock;
> +};
> +
> +#define TPM_DEBUG 0

The TPM_DEBUG stuff should probably be in the directory with the sources, not
in a directory for others to include.

> +#if TPM_DEBUG
> +static inline void dump_options(struct trusted_key_options *o)
> +{
> + pr_info("trusted_key: sealing key type %d\n", o->keytype);
> + pr_info("trusted_key: sealing key handle %0X\n", o->keyhandle);
> + pr_info("trusted_key: pcrlock %d\n", o->pcrlock);
> + pr_info("trusted_key: pcrinfo %d\n", o->pcrinfo_len);
> + print_hex_dump(KERN_INFO, "pcrinfo ", DUMP_PREFIX_NONE,
> + 16, 1, o->pcrinfo, o->pcrinfo_len, 0);
> +}
> +
> +static inline void dump_payload(struct trusted_key_payload *p)
> +{
> + pr_info("trusted_key: key_len %d\n", p->key_len);
> + print_hex_dump(KERN_INFO, "key ", DUMP_PREFIX_NONE,
> + 16, 1, p->key, p->key_len, 0);
> + pr_info("trusted_key: bloblen %d\n", p->blob_len);
> + print_hex_dump(KERN_INFO, "blob ", DUMP_PREFIX_NONE,
> + 16, 1, p->blob, p->blob_len, 0);
> + pr_info("trusted_key: migratable %d\n", p->migratable);
> +}
> +
> +static inline void dump_sess(struct osapsess *s)
> +{
> + print_hex_dump(KERN_INFO, "trusted-key: handle ", DUMP_PREFIX_NONE,
> + 16, 1, &s->handle, 4, 0);
> + pr_info("trusted-key: secret:\n");
> + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE,
> + 16, 1, &s->secret, TPM_HASH_SIZE, 0);
> + pr_info("trusted-key: enonce:\n");
> + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE,
> + 16, 1, &s->enonce, TPM_HASH_SIZE, 0);
> +}
> +
> +static inline void dump_tpm_buf(unsigned char *buf)
> +{
> + int len;
> +
> + pr_info("\ntrusted-key: tpm buffer\n");
> + len = LOAD32(buf, TPM_SIZE_OFFSET);
> + print_hex_dump(KERN_INFO, "", DUMP_PREFIX_NONE, 16, 1, buf, len, 0);
> +}
> +#else
> +static inline void dump_options(struct trusted_key_options *o)
> +{
> +}
> +
> +static inline void dump_payload(struct trusted_key_payload *p)
> +{
> +}
> +
> +static inline void dump_sess(struct osapsess *s)
> +{
> +}
> +
> +static inline void dump_tpm_buf(unsigned char *buf)
> +{
> +}
> +#endif
> +
> +static inline void store8(struct tpm_buf *buf, unsigned char value)
> +{
> + buf->data[buf->len++] = value;
> +}
> +
> +static inline void store16(struct tpm_buf *buf, uint16_t value)
> +{
> + *(uint16_t *) & buf->data[buf->len] = htons(value);
> + buf->len += sizeof(value);
> +}
> +
> +static inline void store32(struct tpm_buf *buf, uint32_t value)
> +{
> + *(uint32_t *) & buf->data[buf->len] = htonl(value);
> + buf->len += sizeof(value);
> +}
> +
> +static inline void storebytes(struct tpm_buf *buf, unsigned char *in, int len)
> +{
> + memcpy(buf->data + buf->len, in, len);
> + buf->len += len;
> +}

Also these look like internal functions which shouldn't be in the global
headers.

David

2010-11-12 17:40:46

[permalink] [raw]

Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type

On Fri, 2010-11-12 at 16:52 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:

Again, thanks for the detailed review!
Willdo on all suggestions with a couple of comments/questions:

> > +#define TPM_MAX_BUF_SIZE 512
> > +#define TPM_TAG_RQU_COMMAND 193
> > +#define TPM_TAG_RQU_AUTH1_COMMAND 194
> > +#define TPM_TAG_RQU_AUTH2_COMMAND 195
> > +#define TPM_TAG_RSP_COMMAND 196
> > +#define TPM_TAG_RSP_AUTH1_COMMAND 197
> > +#define TPM_TAG_RSP_AUTH2_COMMAND 198
> > +#define TPM_NONCE_SIZE 20
> > +#define TPM_HASH_SIZE 20
> > +#define TPM_SIZE_OFFSET 2
> > +#define TPM_RETURN_OFFSET 6
> > +#define TPM_DATA_OFFSET 10
> > +#define TPM_U32_SIZE 4
> > +#define TPM_GETRANDOM_SIZE 14
> > +#define TPM_GETRANDOM_RETURN 14
> > +#define TPM_ORD_GETRANDOM 70
> > +#define TPM_RESET_SIZE 10
> > +#define TPM_ORD_RESET 90
> > +#define TPM_OSAP_SIZE 36
> > +#define TPM_ORD_OSAP 11
> > +#define TPM_OIAP_SIZE 10
> > +#define TPM_ORD_OIAP 10
> > +#define TPM_SEAL_SIZE 87
> > +#define TPM_ORD_SEAL 23
> > +#define TPM_ORD_UNSEAL 24
> > +#define TPM_UNSEAL_SIZE 104
> > +#define SEALKEYTYPE 1
> > +#define SRKKEYTYPE 4
> > +#define SRKHANDLE 0x40000000
> > +#define TPM_ANY_NUM 0xFFFF
> > +#define MAX_PCRINFO_SIZE 64
>
> I suspect some of these should be in somewhere like linux/tpm.h rather than
> here. They're specific to TPM access not TPM key management.

Most of them are not already defined in tpm.h, as they are never used
there, but you are right, some are generic. I'll double check these..

> > +#define TPM_DEBUG 0
>
> The TPM_DEBUG stuff should probably be in the directory with the sources, not
> in a directory for others to include.

Maybe some confusion here - trusted_defined.h is in the sources - only
trusted-type.h is public in include/keys/.

> > +#if TPM_DEBUG
> > +static inline void dump_options(struct trusted_key_options *o)
> > +{
> > + pr_info("trusted_key: sealing key type %d\n", o->keytype);
> > + pr_info("trusted_key: sealing key handle %0X\n", o->keyhandle);
> > + pr_info("trusted_key: pcrlock %d\n", o->pcrlock);
> > + pr_info("trusted_key: pcrinfo %d\n", o->pcrinfo_len);
> > + print_hex_dump(KERN_INFO, "pcrinfo ", DUMP_PREFIX_NONE,
> > + 16, 1, o->pcrinfo, o->pcrinfo_len, 0);
> > +}
> > +
...
> > +
> > +static inline void store16(struct tpm_buf *buf, uint16_t value)
> > +{
> > + *(uint16_t *) & buf->data[buf->len] = htons(value);
> > + buf->len += sizeof(value);
> > +}
> > +
> > +static inline void store32(struct tpm_buf *buf, uint32_t value)
> > +{
> > + *(uint32_t *) & buf->data[buf->len] = htonl(value);
> > + buf->len += sizeof(value);
> > +}
> > +
> > +static inline void storebytes(struct tpm_buf *buf, unsigned char *in, int len)
> > +{
> > + memcpy(buf->data + buf->len, in, len);
> > + buf->len += len;
> > +}
>
> Also these look like internal functions which shouldn't be in the global
> headers.

This is in trusted_defined.h, which is with sources, not in
include/keys/

thanks!
dave

2010-11-12 18:36:40

[permalink] [raw]

Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type

David Safford <[email protected]> wrote:

> > > +#define TPM_DEBUG 0
> >
> > The TPM_DEBUG stuff should probably be in the directory with the sources,
> > not in a directory for others to include.
>
> Maybe some confusion here - trusted_defined.h is in the sources - only
> trusted-type.h is public in include/keys/.

Sorry, you're right.

David

2010-11-12 19:46:22

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> Defines a new kernel key-type called 'encrypted'. Encrypted keys are

Many of the comments I made against patch #3 also apply here. Use 'Define'
rather than 'Defines' here for example.

> index 0000000..e2312e0
> --- /dev/null
> +++ b/include/keys/encrypted-type.h
> @@ -0,0 +1,30 @@
> +/* encrypted-type.h: encrypted-defined key type

Don't include the names of files in the files.

> +struct encrypted_key_payload {
> + struct rcu_head rcu; /* RCU destructor */

That comment is not really needed.

> + char *master_desc; /* datablob: master key name */
> + char *datalen; /* datablob: decrypted key length */
> + void *iv; /* datablob: iv */
> + void *encrypted_data; /* datablob: encrypted key */

But the variable name is 'encrypted_data'...

> + unsigned short datablob_len; /* length of datablob */
> + unsigned short decrypted_datalen; /* decrypted data length */
> + char decrypted_data[0]; /* decrypted data + datablob + hmac */

If any of these are binary data rather than character data, I'd use u8 rather
than char.

> --- /dev/null
> +++ b/security/keys/encrypted_defined.c
> @@ -0,0 +1,816 @@
> +/*
> + * Copyright (C) 2010 IBM Corporation
> + *
> + * Author:
> + * Mimi Zohar <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + *
> + * File: encrypted_defined.c

Don't do that.

> + *
> + * Defines a new kernel key-type called 'encrypted'. Encrypted keys
> + * are kernel generated random numbers, which are encrypted/decrypted
> + * using a 'master' key. The 'master' key can either be a trusted-key or
> + * user-key type. Encrypted keys are created/encrypted/decrypted in the
> + * kernel. Userspace ever only sees/stores encrypted blobs.
> + *
> + * keyctl add "encrypted" "name" "NEW master-key-name keylen" ring
> + * keyctl add "encrypted" "name" "LOAD master-key-name keylen hex_blob" ring
> + * keyctl update keyid "UPDATE master-key-name"

Merge with the documentation file in Documentation/ from patch 3 and stick a
pointer to that file here.

> +static char hash_alg[] = "sha256";
> +static char hmac_alg[] = "hmac(sha256)";

const.

> +static int hash_size = SHA256_DIGEST_SIZE;

??? This is a numeric constant, but you're telling the compiler you want to
store it in the .data section. Is there a reason?

> +static char blkcipher_alg[] = "cbc(aes)";

const.

> +static int ivsize;
> +static int blksize;
> +static int MAX_DATA_SIZE = 4096;
> +static int MIN_DATA_SIZE = 20;

Initialised numeric constants.

> +static int aes_get_sizes(int *ivsize, int *blksize)

It's only used in one place in the file, and it only sets global vars. Why
pass pointers to them?

> +enum {
> + Opt_err = -1, Opt_new = 1, Opt_load,
> + Opt_update, Opt_NEW, Opt_LOAD, Opt_UPDATE
> +};

Why skip 0?

> +static match_table_t key_tokens = {

const.

> + {Opt_new, "new"},
> + {Opt_NEW, "NEW"},
> + {Opt_load, "load"},
> + {Opt_LOAD, "LOAD"},
> + {Opt_update, "update"},
> + {Opt_UPDATE, "UPDATE"},
> + {Opt_err, NULL}

Is case significant? If not, you don't need duplicated constants. Why do you
care about the capitalisation here (and only in a restricted sense) when you
didn't in the trusted keys patch?

> +/* datablob_format - format as an ascii string, before copying to userspace */

I would split this over three lines.

> +static int datablob_format(char __user *buffer,
> + struct encrypted_key_payload *epayload,
> + int asciiblob_len)
> +{
> + char *ascii_buf, *bufp;
> + char *iv = (char *)epayload->iv;

Unnecessary cast. iv is void*.

> + int ret = 0;
> + int len;
> + int i;
> +
> + ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
> + if (!ascii_buf)
> + return -ENOMEM;
> +
> + *(ascii_buf + asciiblob_len) = '\0';

That's what square brackets are for.

> + len = sprintf(ascii_buf, "%s %s ", epayload->master_desc,
> + epayload->datalen);

datalen is not a string.

> +static struct key *request_trusted_key(char *trusted_desc, void **master_key,
> + unsigned int *master_keylen)
> +{
> + struct trusted_key_payload *tpayload;
> + struct key *tkey;
> +
> + tkey = request_key(&key_type_trusted, trusted_desc, NULL);
> + if (IS_ERR(tkey))
> + goto error;
> +
> + tpayload = tkey->payload.data;
> + *master_key = tpayload->key;
> + *master_keylen = tpayload->key_len;

You can't do this. You've defined an update method, so the pointer may change
under you, and the pointee may be destroyed without warning. You've been
using RCU, so you should use that. You need to use rcu_read_{,un}lock() and
rcu_dereference() and you must be aware that the payload may be destroyed from
the moment you drop the RCU read lock.

You're returning a pointer to the key, so you perhaps don't really need to
access the payload at this point.

> +static struct key *request_user_key(char *master_desc, void **master_key,
> + unsigned int *master_keylen)
> +{
> + struct user_key_payload *upayload;
> + struct key *ukey;
> +
> + ukey = request_key(&key_type_user, master_desc, NULL);
> + if (IS_ERR(ukey))
> + goto error;
> +
> + upayload = ukey->payload.data;
> + *master_key = upayload->data;
> + *master_keylen = (unsigned int)upayload->datalen;

Ditto.

> +static int init_desc(struct hash_desc *desc, char *alg)
> +{
> + int ret;
> +
> + desc->tfm = crypto_alloc_hash(alg, 0, CRYPTO_ALG_ASYNC);

Use shash so that you don't have to use the SG interface.

> +static int calc_hmac(char *digest, char *key, int keylen,
> + char *buf, size_t buflen)

Can key be const? Should digest, key and buf be u8* or void* if they're
binary data rather than string data? This sort of thing should perhaps be
percolated through all your functions.

> +static int calc_hash(char *digest, void *buf, int buflen)

Can buf be const?

> +static int get_derived_key(char *derived_key, enum derived_key_type key_type,
> + void *master_key, unsigned int master_keylen)

master_key should be const. master_keylen should perhaps be size_t.

> +static struct key *request_master_key(struct encrypted_key_payload *epayload,
> + void **master_key,
> + unsigned int *master_keylen)
> +{
> + struct key *mkey;
> +
> + mkey = request_trusted_key(epayload->master_desc,
> + master_key, master_keylen);
> + if (IS_ERR(mkey)) {
> + mkey = request_user_key(epayload->master_desc,
> + master_key, master_keylen);
> + if (IS_ERR(mkey)) {
> + pr_info("encrypted_key: trusted/user key %s not found",
> + epayload->master_desc);
> + return mkey;
> + }
> + }
> + dump_master_key(*master_key, *master_keylen);
> + return mkey;
> +}

Why do you allow the master key to be supplied by a user-defined key rather
than requiring a trusted-key unconditionally?

Note that the description of a user-defined key should prefixed to distinguish
it from other random user-defined keys.

> +static int derived_key_encrypt(struct encrypted_key_payload *epayload,
> + void *derived_key, unsigned int derived_keylen)

Should derived_key be const and derived_keylen be size_t?

> +static int datablob_hmac_append(struct encrypted_key_payload *epayload,
> + void *master_key, unsigned int master_keylen)

More const and size_t? There are plenty more places where you should probably
be using const or size_t. I'm not going to list any further

> +static void encrypted_rcu_free(struct rcu_head *rcu)
> +{
> + struct encrypted_key_payload *epayload;
> +
> + epayload = container_of(rcu, struct encrypted_key_payload, rcu);
> + memset(epayload->decrypted_data, 0, epayload->decrypted_datalen);
> + kfree(epayload);

I presume you don't need to release the stuff pointed to by epayload before
the memset because __ekey_init() makes the pointers point at offsets into the
payload blob.

> +static int __init init_encrypted(void)
> +{
> + int ret;
> +
> + ret = register_key_type(&key_type_encrypted);
> + if (ret < 0)
> + return ret;
> + ret = aes_get_sizes(&ivsize, &blksize);
> + return ret;

Merge those two lines into one.

> +static inline void dump_master_key(void *master_key, unsigned int master_keylen)
> +{
> + print_hex_dump(KERN_ERR, "master key: ", DUMP_PREFIX_NONE, 32, 1,
> + master_key, (size_t) master_keylen, 0);

You don't need to cast master_keylen like this. The compiler will do that
automatically. In fact, master_key should be const and master_key_len should
probably be size_t.

> +static inline void dump_decrypted_data(struct encrypted_key_payload
> *epayload)

const.

> +static inline void dump_encrypted_data(struct encrypted_key_payload *epayload,
> + unsigned int encrypted_datalen)

const and size_t. And further instances thereof further down.

David

2010-11-12 21:02:41

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

On Fri, 2010-11-12 at 19:45 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > Defines a new kernel key-type called 'encrypted'. Encrypted keys are
>
> Many of the comments I made against patch #3 also apply here. Use 'Define'
> rather than 'Defines' here for example.

Was expecting your comments. Already started making the changes ...

> > index 0000000..e2312e0
> > --- /dev/null
> > +++ b/include/keys/encrypted-type.h
> > @@ -0,0 +1,30 @@
> > +/* encrypted-type.h: encrypted-defined key type
>
> Don't include the names of files in the files.
>
> > +struct encrypted_key_payload {
> > + struct rcu_head rcu; /* RCU destructor */
>
> That comment is not really needed.
>
> > + char *master_desc; /* datablob: master key name */
> > + char *datalen; /* datablob: decrypted key length */
> > + void *iv; /* datablob: iv */
> > + void *encrypted_data; /* datablob: encrypted key */
>
> But the variable name is 'encrypted_data'...
>
> > + unsigned short datablob_len; /* length of datablob */
> > + unsigned short decrypted_datalen; /* decrypted data length */
> > + char decrypted_data[0]; /* decrypted data + datablob + hmac */
>
> If any of these are binary data rather than character data, I'd use u8 rather
> than char.
>
> > --- /dev/null
> > +++ b/security/keys/encrypted_defined.c
> > @@ -0,0 +1,816 @@
> > +/*
> > + * Copyright (C) 2010 IBM Corporation
> > + *
> > + * Author:
> > + * Mimi Zohar <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, version 2 of the License.
> > + *
> > + * File: encrypted_defined.c
>
> Don't do that.
>
> > + *
> > + * Defines a new kernel key-type called 'encrypted'. Encrypted keys
> > + * are kernel generated random numbers, which are encrypted/decrypted
> > + * using a 'master' key. The 'master' key can either be a trusted-key or
> > + * user-key type. Encrypted keys are created/encrypted/decrypted in the
> > + * kernel. Userspace ever only sees/stores encrypted blobs.
> > + *
> > + * keyctl add "encrypted" "name" "NEW master-key-name keylen" ring
> > + * keyctl add "encrypted" "name" "LOAD master-key-name keylen hex_blob" ring
> > + * keyctl update keyid "UPDATE master-key-name"
>
> Merge with the documentation file in Documentation/ from patch 3 and stick a
> pointer to that file here.
>
> > +static char hash_alg[] = "sha256";
> > +static char hmac_alg[] = "hmac(sha256)";
>
> const.
>
> > +static int hash_size = SHA256_DIGEST_SIZE;
>
> ??? This is a numeric constant, but you're telling the compiler you want to
> store it in the .data section. Is there a reason?
>
> > +static char blkcipher_alg[] = "cbc(aes)";
>
> const.
>
> > +static int ivsize;
> > +static int blksize;
> > +static int MAX_DATA_SIZE = 4096;
> > +static int MIN_DATA_SIZE = 20;
>
> Initialised numeric constants.
>
> > +static int aes_get_sizes(int *ivsize, int *blksize)
>
> It's only used in one place in the file, and it only sets global vars. Why
> pass pointers to them?

agreed

> > +enum {
> > + Opt_err = -1, Opt_new = 1, Opt_load,
> > + Opt_update, Opt_NEW, Opt_LOAD, Opt_UPDATE
> > +};
>
> Why skip 0?
>
> > +static match_table_t key_tokens = {
>
> const.
>
> > + {Opt_new, "new"},
> > + {Opt_NEW, "NEW"},
> > + {Opt_load, "load"},
> > + {Opt_LOAD, "LOAD"},
> > + {Opt_update, "update"},
> > + {Opt_UPDATE, "UPDATE"},
> > + {Opt_err, NULL}
>
> Is case significant? If not, you don't need duplicated constants. Why do you
> care about the capitalisation here (and only in a restricted sense) when you
> didn't in the trusted keys patch?

No, was being consistent with trusted keys, and then uppercase was
removed there, but left here. Will remove.

> > +/* datablob_format - format as an ascii string, before copying to userspace */
>
> I would split this over three lines.
>
> > +static int datablob_format(char __user *buffer,
> > + struct encrypted_key_payload *epayload,
> > + int asciiblob_len)
> > +{
> > + char *ascii_buf, *bufp;
> > + char *iv = (char *)epayload->iv;
>
> Unnecessary cast. iv is void*.
>
> > + int ret = 0;
> > + int len;
> > + int i;
> > +
> > + ascii_buf = kmalloc(asciiblob_len + 1, GFP_KERNEL);
> > + if (!ascii_buf)
> > + return -ENOMEM;
> > +
> > + *(ascii_buf + asciiblob_len) = '\0';
>
> That's what square brackets are for.

Yes, that would be clearer.

> > + len = sprintf(ascii_buf, "%s %s ", epayload->master_desc,
> > + epayload->datalen);
>
> datalen is not a string.

I know I already fixed this. Somehow it crept back in.

> > +static struct key *request_trusted_key(char *trusted_desc, void **master_key,
> > + unsigned int *master_keylen)
> > +{
> > + struct trusted_key_payload *tpayload;
> > + struct key *tkey;
> > +
> > + tkey = request_key(&key_type_trusted, trusted_desc, NULL);
> > + if (IS_ERR(tkey))
> > + goto error;
> > +
> > + tpayload = tkey->payload.data;
> > + *master_key = tpayload->key;
> > + *master_keylen = tpayload->key_len;
>
> You can't do this. You've defined an update method, so the pointer may change
> under you, and the pointee may be destroyed without warning. You've been
> using RCU, so you should use that. You need to use rcu_read_{,un}lock() and
> rcu_dereference() and you must be aware that the payload may be destroyed from
> the moment you drop the RCU read lock.

Thanks for explaining the locking necessary here. Update of an encrypted
key changes the name of the key used to encrypt the data. There are no
guarantees that the key will remain around. So I'm not concerned that
after we release the RCU lock, the key could disappear.

> You're returning a pointer to the key, so you perhaps don't really need to
> access the payload at this point.

Returning a pointer to the key was in order to do the key_put. The
locking probably should be done in request_master_key.

> > +static struct key *request_user_key(char *master_desc, void **master_key,
> > + unsigned int *master_keylen)
> > +{
> > + struct user_key_payload *upayload;
> > + struct key *ukey;
> > +
> > + ukey = request_key(&key_type_user, master_desc, NULL);
> > + if (IS_ERR(ukey))
> > + goto error;
> > +
> > + upayload = ukey->payload.data;
> > + *master_key = upayload->data;
> > + *master_keylen = (unsigned int)upayload->datalen;
>
> Ditto.
>
> > +static int init_desc(struct hash_desc *desc, char *alg)
> > +{
> > + int ret;
> > +
> > + desc->tfm = crypto_alloc_hash(alg, 0, CRYPTO_ALG_ASYNC);
>
> Use shash so that you don't have to use the SG interface.

Will look into it.

> > +static int calc_hmac(char *digest, char *key, int keylen,
> > + char *buf, size_t buflen)
>
> Can key be const? Should digest, key and buf be u8* or void* if they're
> binary data rather than string data? This sort of thing should perhaps be
> percolated through all your functions.

Yes, will review the types used through out, adding const when possible.

> > +static int calc_hash(char *digest, void *buf, int buflen)
>
> Can buf be const?
>
> > +static int get_derived_key(char *derived_key, enum derived_key_type key_type,
> > + void *master_key, unsigned int master_keylen)
>
> master_key should be const. master_keylen should perhaps be size_t.
>
> > +static struct key *request_master_key(struct encrypted_key_payload *epayload,
> > + void **master_key,
> > + unsigned int *master_keylen)
> > +{
> > + struct key *mkey;
> > +
> > + mkey = request_trusted_key(epayload->master_desc,
> > + master_key, master_keylen);
> > + if (IS_ERR(mkey)) {
> > + mkey = request_user_key(epayload->master_desc,
> > + master_key, master_keylen);
> > + if (IS_ERR(mkey)) {
> > + pr_info("encrypted_key: trusted/user key %s not found",
> > + epayload->master_desc);
> > + return mkey;
> > + }
> > + }
> > + dump_master_key(*master_key, *master_keylen);
> > + return mkey;
> > +}
>
> Why do you allow the master key to be supplied by a user-defined key rather
> than requiring a trusted-key unconditionally?

This is for systems without a TPM. The logic needs to exist, whether it
is here or in EVM. By doing it here, a user could provide a passphrase
in the initramfs, which is used to decrypt the encrypted key.

> Note that the description of a user-defined key should prefixed to distinguish
> it from other random user-defined keys.
>
> > +static int derived_key_encrypt(struct encrypted_key_payload *epayload,
> > + void *derived_key, unsigned int derived_keylen)
>
> Should derived_key be const and derived_keylen be size_t?
>
> > +static int datablob_hmac_append(struct encrypted_key_payload *epayload,
> > + void *master_key, unsigned int master_keylen)
>
> More const and size_t? There are plenty more places where you should probably
> be using const or size_t. I'm not going to list any further
>
> > +static void encrypted_rcu_free(struct rcu_head *rcu)
> > +{
> > + struct encrypted_key_payload *epayload;
> > +
> > + epayload = container_of(rcu, struct encrypted_key_payload, rcu);
> > + memset(epayload->decrypted_data, 0, epayload->decrypted_datalen);
> > + kfree(epayload);
>
> I presume you don't need to release the stuff pointed to by epayload before
> the memset because __ekey_init() makes the pointers point at offsets into the
> payload blob.

right

> > +static int __init init_encrypted(void)
> > +{
> > + int ret;
> > +
> > + ret = register_key_type(&key_type_encrypted);
> > + if (ret < 0)
> > + return ret;
> > + ret = aes_get_sizes(&ivsize, &blksize);
> > + return ret;
>
> Merge those two lines into one.

ok

> > +static inline void dump_master_key(void *master_key, unsigned int master_keylen)
> > +{
> > + print_hex_dump(KERN_ERR, "master key: ", DUMP_PREFIX_NONE, 32, 1,
> > + master_key, (size_t) master_keylen, 0);
>
> You don't need to cast master_keylen like this. The compiler will do that
> automatically. In fact, master_key should be const and master_key_len should
> probably be size_t.
>
> > +static inline void dump_decrypted_data(struct encrypted_key_payload
> > *epayload)
>
> const.
>
> > +static inline void dump_encrypted_data(struct encrypted_key_payload *epayload,
> > + unsigned int encrypted_datalen)
>
> const and size_t. And further instances thereof further down.
>
> David

thanks,

Mimi

2010-11-12 21:24:20

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> > Why do you allow the master key to be supplied by a user-defined key rather
> > than requiring a trusted-key unconditionally?
>
> This is for systems without a TPM. The logic needs to exist, whether it
> is here or in EVM. By doing it here, a user could provide a passphrase
> in the initramfs, which is used to decrypt the encrypted key.

I thought that might be the case. In which case, it might be better to allow
someone to add a trusted key, supplying both encrypted and unencrypted
versions of the data so that the TPM need not be consulted. You might want to
mark such a key so that it can be seen when it is dumped.

But if you're going to use a user-defined key, you really need to prefix the
description with something suitable.

David

2010-11-12 21:25:00

by Rajiv Andrade

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

Hi Dave,

On 11/12/2010 12:48 PM, David Safford wrote:

> On Fri, 2010-11-12 at 14:11 +0000, David Howells wrote:
>> Mimi Zohar<[email protected]> wrote:
>>
>>>>> + module_put(chip->dev->driver->owner);
>>>> Where's the corresponding module_get()? I suspect this should be wrapped to
>>>> match tpm_chip_find_get().
>>>>
>>>> David
>>> The module_get() is in tpm_chip_find_get(), which is just a helper.
>>> (It's used this way throughout tpm.c)
>> I'd make a function tpm_chip_find_put() just to wrap module_put() and then
>> place it with tpm_chip_find_get() in the sources. That makes it easier to see
>> what's going on.
> Or alternately rename the helper to tpm_chip_find_and_module_get()?
> In either case, this is really up to Rajiv (the maintainer for tpm.c
> who has already acked this patch), as this usage already appears in
> other places in tpm.c. There would have to be a separate patch fixing
> this for all of the instances. Rajiv, your thoughts?
>
> dave
Rename the helper to tpm_chip_find_and_module_get() solves the naming issue in a simpler and better way in my opinion.

Thanks,
Rajiv

2010-11-12 22:07:31

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

On Fri, 2010-11-12 at 19:24 -0200, Rajiv Andrade wrote:
>
> Hi Dave,
>
> On 11/12/2010 12:48 PM, David Safford wrote:
>
> > On Fri, 2010-11-12 at 14:11 +0000, David Howells wrote:
> >> Mimi Zohar<[email protected]> wrote:
> >>
> >>>>> + module_put(chip->dev->driver->owner);
> >>>> Where's the corresponding module_get()? I suspect this should be wrapped to
> >>>> match tpm_chip_find_get().
> >>>>
> >>>> David
> >>> The module_get() is in tpm_chip_find_get(), which is just a helper.
> >>> (It's used this way throughout tpm.c)
> >> I'd make a function tpm_chip_find_put() just to wrap module_put() and then
> >> place it with tpm_chip_find_get() in the sources. That makes it easier to see
> >> what's going on.
> > Or alternately rename the helper to tpm_chip_find_and_module_get()?
> > In either case, this is really up to Rajiv (the maintainer for tpm.c
> > who has already acked this patch), as this usage already appears in
> > other places in tpm.c. There would have to be a separate patch fixing
> > this for all of the instances. Rajiv, your thoughts?
> >
> > dave
> Rename the helper to tpm_chip_find_and_module_get() solves the naming issue
> in a simpler and better way in my opinion.
>
> Thanks,
> Rajiv

David, does this look ok to you? If so, I will do two patches, one to
fix the helper name throughout the existing tpm.c, and then a new
version of the tpm_send patch which uses the new name.

thanks
dave

2010-11-12 22:12:03

[permalink] [raw]

Subject: Re: [PATCH v1.3 2/4] key: add tpm_send command

David Safford <[email protected]> wrote:

> David, does this look ok to you? If so, I will do two patches, one to
> fix the helper name throughout the existing tpm.c, and then a new
> version of the tpm_send patch which uses the new name.

I prefer my suggestion: Wrapping the module_put() up so that you don't see it
directly. Then you don't need to alter tpm_chip_find_get(). I'll argue that
you don't need to know how tpm_chips are got/put, except in the code that
wraps it.

David

2010-11-14 00:48:34

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

On Fri, 2010-11-12 at 21:23 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > > Why do you allow the master key to be supplied by a user-defined key rather
> > > than requiring a trusted-key unconditionally?
> >
> > This is for systems without a TPM. The logic needs to exist, whether it
> > is here or in EVM. By doing it here, a user could provide a passphrase
> > in the initramfs, which is used to decrypt the encrypted key.
>
> I thought that might be the case. In which case, it might be better to allow
> someone to add a trusted key, supplying both encrypted and unencrypted
> versions of the data so that the TPM need not be consulted. You might want to
> mark such a key so that it can be seen when it is dumped.

At least to me, the name 'trusted' implies some form of HW.

> But if you're going to use a user-defined key, you really need to prefix the
> description with something suitable.
>
> David

Agreed. So instead of:
keyctl add encrypted name "new master-key-name keylen" ring

the description would be prefixed with the key type like:
keyctl add encrypted name "new trusted|user master-key-name keylen" ring

thanks,

Mimi

2010-11-15 16:19:23

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> > I thought that might be the case. In which case, it might be better to
> > allow someone to add a trusted key, supplying both encrypted and
> > unencrypted versions of the data so that the TPM need not be consulted.
> > You might want to mark such a key so that it can be seen when it is
> > dumped.
>
> At least to me, the name 'trusted' implies some form of HW.

In many ways, I think that the type and description describe the purpose of
the key, not its source or derivation.

> > But if you're going to use a user-defined key, you really need to prefix
> > the description with something suitable.
>
> Agreed. So instead of:
> keyctl add encrypted name "new master-key-name keylen" ring
>
> the description would be prefixed with the key type like:
> keyctl add encrypted name "new trusted|user master-key-name keylen" ring

I don't think you understood what I meant. If you look at the following
function:

+static struct key *request_master_key(struct encrypted_key_payload *epayload,
+ void **master_key,
+ unsigned int *master_keylen)
+{
+ struct key *mkey;
+
+ mkey = request_trusted_key(epayload->master_desc,
+ master_key, master_keylen);
+ if (IS_ERR(mkey)) {
+ mkey = request_user_key(epayload->master_desc,
+ master_key, master_keylen);
+ if (IS_ERR(mkey)) {
+ pr_info("encrypted_key: trusted/user key %s not found",
+ epayload->master_desc);
+ return mkey;
+ }
+ }
+ dump_master_key(*master_key, *master_keylen);
+ return mkey;
+}

In the bit where you go for a user key (having failed to get a trusted key),
you should prefix the description here (or in request_user_key()) with
something like "trusted:". Then you don't need to change the user interface.

David

2010-11-15 19:36:07

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

On Mon, 2010-11-15 at 16:18 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:

> > > But if you're going to use a user-defined key, you really need to prefix
> > > the description with something suitable.
> >
> > Agreed. So instead of:
> > keyctl add encrypted name "new master-key-name keylen" ring
> >
> > the description would be prefixed with the key type like:
> > keyctl add encrypted name "new trusted|user master-key-name keylen" ring
>
> I don't think you understood what I meant. If you look at the following
> function:

> +static struct key *request_master_key(struct encrypted_key_payload *epayload,
> + void **master_key,
> + unsigned int *master_keylen)
> +{
> + struct key *mkey;
> +
> + mkey = request_trusted_key(epayload->master_desc,
> + master_key, master_keylen);
> + if (IS_ERR(mkey)) {
> + mkey = request_user_key(epayload->master_desc,
> + master_key, master_keylen);
> + if (IS_ERR(mkey)) {
> + pr_info("encrypted_key: trusted/user key %s not found",
> + epayload->master_desc);
> + return mkey;
> + }
> + }
> + dump_master_key(*master_key, *master_keylen);
> + return mkey;
> +}
>
> In the bit where you go for a user key (having failed to get a trusted key),
> you should prefix the description here (or in request_user_key()) with
> something like "trusted:". Then you don't need to change the user interface.
>
> David

Am assuming you mean something like this:

keyctl add encrypted name "new trusted:master-key-name keylen" ring
keyctl add encrypted name "new user:master-key-name keylen" ring

and, as you said, works without changing the API.

thanks,

Mimi

2010-11-16 14:09:13

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> Am assuming you mean something like this:
>
> keyctl add encrypted name "new trusted:master-key-name keylen" ring
> keyctl add encrypted name "new user:master-key-name keylen" ring
>
> and, as you said, works without changing the API.

No, that's not what I mean. I maeant that when your internal functions look
for the user key, they should preface the description with a prefix.

It should be handled in request_user_key() or request_master_key(). The
description given to request_trusted_key() should have the prefix applied
there. There's no need to mention it at all in the encrypted key add_key
command line.

David

2010-11-16 14:38:47

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

On Tue, 2010-11-16 at 14:08 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > Am assuming you mean something like this:
> >
> > keyctl add encrypted name "new trusted:master-key-name keylen" ring
> > keyctl add encrypted name "new user:master-key-name keylen" ring
> >
> > and, as you said, works without changing the API.
>
> No, that's not what I mean. I maeant that when your internal functions look
> for the user key, they should preface the description with a prefix.
>
> It should be handled in request_user_key() or request_master_key(). The
> description given to request_trusted_key() should have the prefix applied
> there. There's no need to mention it at all in the encrypted key add_key
> command line.
>
> David

I actually like keyctl requiring 'trusted:' or 'user:'. Forcing the
user to indicate which type of key they want, is actually good - no
misunderstandings. Another benefit, would be allowing 'keyctl update' to
update the key description, not the key type.

Mimi

2010-11-16 18:32:53

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> I actually like keyctl requiring 'trusted:' or 'user:'. Forcing the
> user to indicate which type of key they want, is actually good - no
> misunderstandings.

You still need to prefix the description of a user-defined key so that you
don't collide with other people who're also using user-defined keys for random things.

> Another benefit, would be allowing 'keyctl update' to update the key
> description, not the key type.

You mean you want to change the description on a key?

David

2010-11-16 18:54:57

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

On Tue, 2010-11-16 at 17:50 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > I actually like keyctl requiring 'trusted:' or 'user:'. Forcing the
> > user to indicate which type of key they want, is actually good - no
> > misunderstandings.
>
> You still need to prefix the description of a user-defined key so that you
> don't collide with other people who're also using user-defined keys for random things.

ok

> > Another benefit, would be allowing 'keyctl update' to update the key
> > description, not the key type.
>
> You mean you want to change the description on a key?
>
> David

No, this just updates the name of the key used to encrypt/decrypt the
encrypted key. For example, the encrypted key evm-key is initially
encrypted/decrypted using 'kmk-trusted'. After the update, it is
encrypted/decrypted with 'kmk'. Both now are trusted keys.

$ keyctl show
Session Keyring
-3 --alswrv 500 500 keyring: _ses
117908125 --alswrv 500 -1 \_ keyring: _uid.500
501967942 --alswrv 500 500 \_ trusted: kmk-trusted
317523177 --alswrv 500 500 \_ encrypted: evm-key
666437381 --alswrv 500 500 \_ trusted: kmk
$ keyctl print 317523177
trusted:kmk-trusted 32 ca0ebb83594f14781460 ...

$ keyctl update 317523177 "update trusted:kmk"
$ keyctl print 317523177
trusted:kmk 32 ca0ebb83594f1478146 ....

Mimi

2010-11-16 18:58:56

[permalink] [raw]

Subject: Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Mimi Zohar <[email protected]> wrote:

> No, this just updates the name of the key used to encrypt/decrypt the
> encrypted key. For example, the encrypted key evm-key is initially
> encrypted/decrypted using 'kmk-trusted'. After the update, it is
> encrypted/decrypted with 'kmk'. Both now are trusted keys.

Gotcha. The problem is that there are two many different objects in the
system called 'keys':-)

David

2010-11-16 20:43:58