[U-Boot] (Mixed security state) Re: [PATCH 1/6] tpm: add AUTH1 cmds for LoadKey2 and GetPubKey

[Pfau, Reinhard]

Hi,

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Tom Rini
> Sent: Monday, April 22, 2013 8:37 PM
> To: Eibach, Dirk
> Cc: u-boot at lists.denx.de
> Subject: (Mixed security state) Re: [U-Boot] [PATCH 1/6] tpm: 
> add AUTH1 cmds for LoadKey2 and GetPubKey
> 
> On Mon, Apr 22, 2013 at 01:06:40PM +0200, Dirk Eibach wrote:
> 
> > From: Reinhard Pfau <pfau at gdsys.de>
> > 
> > Extend the tpm library with support for single authorized 
> (AUTH1) commands
> > as specified in the TCG Main Specification 1.2. (The 
> internally used helper
> > functions are implemented in a way that they could also be 
> used for double
> > authorized commands if someone needs it.)
> > 
> > Provide enums with the return codes from the TCG Main specification.
> > 
> > For now only a single OIAP session is supported.
> > 
> > OIAP authorized version of the commands TPM_LoadKey2 and 
> TPM_GetPubKey are
> > provided. Both features are available using the 'tpm' command, too.
> > 
> > Authorized commands are enabled with 
> CONFIG_TPM_AUTH_SESSIONS. (Note that
> > this also requires CONFIG_SHA1 to be enabled.)
> > 
> > Signed-off-by: Reinhard Pfau <pfau at gdsys.de>
> > 
> > Signed-off-by: Dirk Eibach <eibach at gdsys.de>
> 
> I'm glad to see this as it helps further the TPM related discussions.
> 
> [snip]
> > +#ifdef CONFIG_TPM_AUTH_SESSIONS
> > +
> > +static int do_tpm_oiap(cmd_tbl_t *cmdtp, int flag,
> > +		int argc, char * const argv[])
> 
> checkpatch warning.  Please run ./tools/checkpatch.pl and fix
> everything.

OK; will be done.

> 
> [snip]
> > +	uint8_t usage_auth[20];
> [snip]
> > +	if (strlen(argv[4]) != 40)
> > +		return CMD_RET_FAILURE;
> [snip]
> > +	uint8_t usage_auth[20];
> > +	uint8_t pub_key_buffer[288];
> 
> These are just a few examples of array sizes that clearly have a
> specific meaning (288 == TPM_PUBKEY_MAX_LENGTH I think), that 
> we should
> be using the name for, for clarity.

Like we did in lib/tpm.c; sounds like a good idea. Will be done, too.

> 
> [snip]
> >  /**
> > + * TPM return codes as defined in the TCG Main specification
> > + * (TPM Main Part 2 Structures; Specification version 1.2)
> > + */
> > +enum tpm_return_code {
> > +	TPM_BASE	= 0x00000000,
> > +	TPM_NON_FATAL	= 0x00000800,
> > +	TPM_SUCCESS	= TPM_BASE,
> > +	/* TPM-defined fatal error codes */
> > +	TPM_AUTHFAIL			= TPM_BASE +  1,
> [snip]
> > +	TPM_BADINDEX			= TPM_BASE +  2,
> 
> I don't like this form, and it's not what we usually use.  It 
> should be,
> roughly:
> enum tpm_return_code {
>  TPM_SUCCESS = 0,
>  /* TPM-defined fatal error codes. */
>  TPM_BAD_PARAMETER,
>  TPM_AUDITFAILURE,
>  ...
>  /* TPM-defined non-fatal error codes. */
>  TPM_RETRY = 0x800,
>  TPM_NEEDS_SELFTEST,
>  ...
> }

Well, the way I wrote the constants is intentionally since the return
codes are defined like this in the TCG specification.
In the spec the return codes are found in a table with name and value;
and the value is expressed as sum based on TPM_BASE.
(See TCG published spec: "TPM Main Part 2 TPM Structures; Specification
version 1.2" chapter 16 ("Return Codes").)

This way it might be easier to keep the constants in sync with (future)
versions of the TCG spec :-)
So I would like to keep it as it is.


Greetings,
Reinhard.