Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

quoted text
> +keyctl print returns an ascii hex copy of the sealed key, which is in standard

I'd quote 'keyctl print' just so it's obvious where the command ends and the
descriptive text starts.

quoted text
> +Usage:
> +  keyctl add encrypted name "new key-type:master-key-name keylen" ring
> +  keyctl add encrypted name "load key-type:master-key-name keylen hex_blob" ring
> +  keyctl update keyid "update key-type:master-key-name"
> +
> +where 'key-type' is either 'trusted' or 'user'.

I recommend adding some example commands with all the arguments substituted.
Nothing helps get to grip with an API like knowing what a command is supposed
to look like when it's actually used.

quoted text
> +static int trusted_tpm_send(u32 chip_num, unsigned char *cmd, int buflen)

There are still a lot of places in here where you should probably be using
const and size_t.

quoted text
> +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);

Using kzalloc() rather than kmalloc() is a waste of time, I'd've thought.
It's a temporary buffer.  Does it really need to be precleared?

quoted text
> +	ret = tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash);
> +	return ret;

Merge.

quoted text
> +static int trusted_update(struct key *key, const void *data, size_t datalen)
> +{
> ...
> +	*(datablob + datalen) = '__PLACEHOLDER__5_';

That's what [] is for.

quoted text
> +	if (new_o)
> +		kfree(new_o);

kfree() can handle a NULL pointer.

quoted text
> +	if (new_o->pcrlock)
> +		ret = pcrlock(new_o->pcrlock);
> +	rcu_assign_pointer(key->payload.data, new_p);
> +	call_rcu(&p->rcu, trusted_rcu_free);

Should there be a check for pcrlock() failure?

quoted text
> +/* not already defined in tpm.h - specific to this use */
> +#define TPM_TAG_RQU_COMMAND		193
> +#define TPM_TAG_RQU_AUTH1_COMMAND	194
> ...

Values defined for TPM hardware access really ought to be in a separate file
in include/linux/.  They aren't strictly specific to the trusted key
implementation here; that may be the only user currently in the kernel, but
that doesn't mean there can't be another user.

David
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/