linux-kernel - Re: [PATCH v1.4 4/5] keys: add new trusted key-type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 19 Nov 2010 16:23:21 +0000
From:	David Howells <dhowells@...hat.com>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	dhowells@...hat.com, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org, keyrings@...ux-nfs.org,
	linux-crypto@...r.kernel.org,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	James Morris <jmorris@...ei.org>,
	David Safford <safford@...son.ibm.com>,
	Rajiv Andrade <srajiv@...ux.vnet.ibm.com>,
	Mimi Zohar <zohar@...ibm.com>
Subject: Re: [PATCH v1.4 4/5] keys: add new trusted key-type

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

> +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.

> +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.

> +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.

> +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?

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

Merge.

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

That's what [] is for.

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

kfree() can handle a NULL pointer.

> +	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?

> +/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/