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

["Serge E. Hallyn" <serge@hallyn.com>]

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):

Looks fine to me, and very useful.

Acked-by: Serge E. Hallyn <serge@hallyn.com>

(for 1-3, haven't looked at 4 yet and won't tonight)

> +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/unsealing keys
> +	  in the kernel. Trusted keys are TPM generated random numbers
> +	  symmetric keys, RSA sealed by the TPM, and only unsealed by the
> +	  TPM, if boot PCRs and other criteria match.  Userspace ever only
> +	  sees/stores encrypted blobs.

"
This option provides support for creating, sealing, and unsealing keys
in the kernel.  Trusted keys are ramdon symmetric keys created
RSA-sealed, and stored in the TPM.  The TPM only unseals the keys if the
boot PCRs and other criteria match.  Userspace can only ever see
encrypted blobs.
"

> diff --git a/security/keys/trusted_defined.c b/security/keys/trusted_defined.c
> new file mode 100644
> index 0000000..0bd935f
> --- /dev/null
> +++ b/security/keys/trusted_defined.c
> @@ -0,0 +1,999 @@
> +/*
> + * Copyright (C) 2010 IBM Corporation
> + *
> + * Author:
> + * David Safford <safford@us.ibm.com>
> + *
> + * 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: trusted_defined.c

(don't put filenames in comments :)

> + * Defines a new kernel key-type called 'trusted'. Trusted keys are
> + * TPM generated random numbers, RSA sealed by the TPM, and only unsealed
> + * by the TPM, if boot PCRs and other criteria match. Trusted keys are
> + * created/sealed/unsealed in the kernel. Userspace ever only sees/stores
> + * encrypted blobs.
> + *
> + * Keys are sealed under the SRK, which must have the default
> + * authorization value (20 zeros).

What does this mean?  They have to be sealed by a key that starts
with 20 zeros?  (of course it doesn't mean that, but i don't understand
what it does mean :)

> This can be set at takeownership
> + * time with the trouser's utility "tpm_takeownership -u -z".
> + *
> + * Usage:
> + *   keyctl add trusted name "NEW keylen [hex_pcrinfo]" ring
> + *   keyctl add trusted name "LOAD hex_blob" ring
> + *   keyctl update key "UPDATE hex_pcrinfo"
> + *   keyctl print keyid
> + * 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/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);
> +	crypto_hash_update(&desc, sg, datalen);
> +	crypto_hash_final(&desc, digest);
> +	crypto_free_hash(desc.tfm);
> +	return rc;

In later functions where rc can only be 0, you 'return 0;', but here
you return rc.  Is that an oversight, or is there something actually
wrong here (like a missing hunk)?

There are also several places where you:

> +	datablob = kzalloc(datalen + 1, GFP_KERNEL);
> +	if (!datablob)
> +		return -ENOMEM;
> +	memcpy(datablob, data, datalen);

don't need to kzalloc.

-serge