From: David Howells <dhowells@redhat.com>
To: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: dhowells@redhat.com, linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, keyrings@linux-nfs.org,
linux-crypto@vger.kernel.org,
Jason Gunthorpe <jgunthorpe@obsidianresearch.com>,
James Morris <jmorris@namei.org>,
David Safford <safford@watson.ibm.com>,
Rajiv Andrade <srajiv@linux.vnet.ibm.com>,
Mimi Zohar <zohar@us.ibm.com>
Subject: Re: [PATCH v1.3 3/4] keys: add new trusted key-type
Date: Fri, 12 Nov 2010 16:52:59 +0000 [thread overview]
Message-ID: <24801.1289580779@redhat.com> (raw)
In-Reply-To: <1289404309-15955-4-git-send-email-zohar@linux.vnet.ibm.com>
Mimi Zohar <zohar@linux.vnet.ibm.com> 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
next prev parent reply other threads:[~2010-11-12 16:52 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-10 15:51 [PATCH v1.3 0/4] keys: trusted and encrypted keys Mimi Zohar
2010-11-10 15:51 ` [PATCH v1.3 1/4] lib: hex2bin converts ascii hexadecimal string to binary Mimi Zohar
2010-11-10 15:51 ` [PATCH v1.3 2/4] key: add tpm_send command Mimi Zohar
2010-11-10 15:51 ` [PATCH v1.3 3/4] keys: add new trusted key-type Mimi Zohar
2010-11-10 15:51 ` [PATCH v1.3 4/4] keys: add new key-type encrypted Mimi Zohar
2010-11-11 19:48 ` [PATCH v1.3 1/4] lib: hex2bin converts ascii hexadecimal string to binary David Howells
2010-11-11 22:23 ` Mimi Zohar
2010-11-11 19:48 ` [PATCH v1.3 2/4] key: add tpm_send command David Howells
2010-11-11 22:25 ` Mimi Zohar
2010-11-12 14:11 ` David Howells
2010-11-12 14:48 ` David Safford
2010-11-12 21:24 ` Rajiv Andrade
2010-11-12 22:06 ` David Safford
2010-11-12 22:11 ` David Howells
2010-11-17 13:12 ` Rajiv Andrade
2010-11-11 21:57 ` [PATCH v1.3 3/4] keys: add new trusted key-type David Howells
2010-11-12 12:58 ` David Safford
2010-11-12 16:52 ` David Howells [this message]
2010-11-12 17:39 ` David Safford
2010-11-12 18:36 ` David Howells
2010-11-12 19:45 ` [PATCH v1.3 4/4] keys: add new key-type encrypted David Howells
2010-11-12 21:02 ` Mimi Zohar
2010-11-12 21:23 ` David Howells
2010-11-14 0:33 ` Mimi Zohar
2010-11-15 16:18 ` David Howells
2010-11-15 19:35 ` Mimi Zohar
2010-11-16 14:08 ` David Howells
2010-11-16 14:38 ` Mimi Zohar
2010-11-16 17:50 ` David Howells
2010-11-16 18:54 ` Mimi Zohar
2010-11-16 18:58 ` David Howells
2010-11-16 20:43 ` Mimi Zohar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=24801.1289580779@redhat.com \
--to=dhowells@redhat.com \
--cc=jgunthorpe@obsidianresearch.com \
--cc=jmorris@namei.org \
--cc=keyrings@linux-nfs.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=safford@watson.ibm.com \
--cc=srajiv@linux.vnet.ibm.com \
--cc=zohar@linux.vnet.ibm.com \
--cc=zohar@us.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.