lkml.org 
[lkml]   [2010]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v1.3 3/4] keys: add new trusted key-type
    Date
    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


    \
     
     \ /
      Last update: 2010-11-12 17:57    [W:0.042 / U:151.552 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site