Re: [SeaBIOS] [PATCH 3/3] tpm: Add a menu for TPM configuration

[Stefan Berger]

"Kevin O'Connor"  wrote on 11/30/2015 10:05:22 AM:
> From: "Kevin O'Connor" 
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: [e-mail], Stefan Berger 
> Date: 11/30/2015 10:05 AM
> Subject: Re: [PATCH 3/3] tpm: Add a menu for TPM configuration
>
> On Sun, Nov 29, 2015 at 11:49:43PM -0500, Stefan Berger wrote:
> > From: Stefan Berger
> >
> > This patch adds an new menu entry to the main menu. This menu item

enables
> > the user to enter a TPM control menu which allows control of those

aspects
> > of the TPM's state that can only be controlled while in the firmware
> > and while physical presence can be asserted.
>
> [...]
> > --- a/src/std/tcg.h
> > +++ b/src/std/tcg.h
>
> [...]
> > @@ -325,4 +348,22 @@ struct tpm_res_sha1complete {
> >      u8     hash[20];
> >  } PACKED;
> >
> > +#define TPM_STATE_ENABLED 1
> > +#define TPM_STATE_ACTIVE 2
> > +#define TPM_STATE_OWNED 4
> > +#define TPM_STATE_OWNERINSTALL 8
> >
> > +/*
> > physical presence interface
> > */
> >
> > +#define TPM_PPI_OP_NOOP 0
> > +#define TPM_PPI_OP_ENABLE 1
> > +#define TPM_PPI_OP_DISABLE 2
> > +#define TPM_PPI_OP_ACTIVATE 3
> > +#define TPM_PPI_OP_DEACTIVATE 4
> > +#define TPM_PPI_OP_CLEAR 5
> > +#define TPM_PPI_OP_SET_OWNERINSTALL_TRUE 8
> > +#define TPM_PPI_OP_SET_OWNERINSTALL_FALSE 9
>
> Are the above definitions part of the standard, or internal to the
> implementation?  If the latter, they should go into tcgbios.[ch]

These are actually the 'message codes' from the physical presence 
interface spec that can be sent from the OS
to the BIOS and on which the BIOS is supposed to act upon reboot. You may 
remember the ACPI patches I had for QEMU
where ACPI would write one of the above number into an allocated memory 
area for the BIOS to find. I also built
the menu using those 'message codes'.
> > --- a/src/tcgbios.c
> > +++ b/src/tcgbios.c
> > @@ -23,6 +23,8 @@
> >  #include "string.h" // checksum
> >  #include "tcgbios.h"// tpm_*, prototypes
> >  #include "util.h" // printf, get_keystroke
> > +#include "malloc.h" // malloc_*
> > +#include "stacks.h" // wait_threads
>
> Doesn't look like malloc_x() is used in this code.
>
> [...]
> > +typedef struct {
> > u8  op;
> >
> > +} tpm_bios_cfg;
>
> What is the purpose of this struct?

This datatype is used to send messages from menu item selections to the 
part that processes above 'message codes'.
> > +extern void reset_vector(void) __noreturn;
>
> The code should use reset() (defined in stacks.h).  Directly calling
> reset_vector() in 32bit mode isn't strictly correct.

Ok, will change it. (Worked so far...)

Stefan
> Looks good to me.
> -Kevin