Re: [SeaBIOS] [PATCH 3/5] Add support for harware physical presence

[Stefan Berger]

"Kevin O'Connor"  wrote on 01/06/2016 03:20:56 PM:
> On Wed, Jan 06, 2016 at 01:15:55PM -0500, Stefan Berger wrote:
> > From: Stefan Berger
> >
> > Extend assert_physical_presence with checks for hardware physical

presence
> > support. If no hardware physical presence is asserted and the SW

assertion
> > is disable, -1 is returned.
> > Signed-off-by: Stefan Berger
> >
> > src/std/tcg.h |  2 ++
> >  src/tcgbios.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>
> +++++++++++--
> > 2 files changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/std/tcg.h b/src/std/tcg.h
> > index 9f7f021..00be533 100644
> > --- a/src/std/tcg.h
> > +++ b/src/std/tcg.h
> > @@ -70,6 +70,7 @@
> >
> > /* TPM command error codes */
> >  #define TPM_INVALID_POSTINIT             0x26
> > +#define TPM_BAD_PRESENCE                 0x2d
> >  #define TPM_BAD_LOCALITY                 0x3d
> >
> > /* TPM command tags */
> > @@ -285,6 +286,7 @@ enum permFlagsIndex {
> >      PERM_FLAG_IDX_ALLOW_MAINTENANCE,
> >      PERM_FLAG_IDX_PHYSICAL_PRESENCE_LIFETIME_LOCK,
> >      PERM_FLAG_IDX_PHYSICAL_PRESENCE_HW_ENABLE,
> > PERM_FLAG_IDX_PHYSICAL_PRESENCE_CMD_ENABLE,
> >
> > };
> >
> > diff --git a/src/tcgbios.c b/src/tcgbios.c
> > index d6a8495..6a6b6b0 100644
> > --- a/src/tcgbios.c
> > +++ b/src/tcgbios.c
> > @@ -446,7 +446,7 @@ err_exit:
> >      return -1;
> >  }
> >
> > -static u32
> > +static int
> >  read_stclear_flags(char *buf, int buf_len)
> >  {
> >      memset(buf, 0, buf_len);
> > @@ -480,7 +480,32 @@ read_permanent_flags(char *buf, int buf_len)
> >      return 0;
> >  }
> >
> > -static u32
> > +static int
> > +has_hw_physical_presence(struct tpm_permanent_flags *pf, int

*has_hw_pp)
> > +{
> > u32 ordinal;
> > /* We cannot read hardware physical presence from a flag;
> > * it has to be inferred from the error code to a command that
> > * needs physical presence
> > */
> > if (pf->flags[PERM_FLAG_IDX_DISABLE])
> >    ordinal = TPM_ORD_PhysicalDisable;
> > else
> >    ordinal = TPM_ORD_PhysicalEnable;
> > int ret = tpm_send_cmd(0, ordinal, NULL, 0,

TPM_DURATION_TYPE_SHORT);
> I'm leery of code that automatically issues a command that nominally
> alters non-volatile memory as I fear it could cause the hardware to
> wear out.  So, I'd avoid doing this unless the above is definitely not
> an issue.

It's implementation-dependent what the TPM will do once a permanent flag 
is set to the
same value it already is. It may write it back into NVRAM or not.
> If you want the menu to also be usable on machines with hardware
> physical presence, then I'm fine with enabling the menu after just
> checking that HW_ENABLE is true.

In this case we may encounter errors if hardware physical presence is not 
actually set.
> [...]
> > +static int
> >  assert_physical_presence(int verbose)
> >  {
> >      struct tpm_stclear_flags stcf;
> > @@ -492,6 +517,38 @@ assert_physical_presence(int verbose)
> >          /* physical presence already asserted */
> >          return 0;
>
> I don't think we need to read stclear flags here - I think it would be
> simpler to just issue PhysicalPresence_PRESENT.  If it succeeds then
> we've successfully asserted physical presence, and if it fails then go
> on to read permanent flags.

Ok.
> > struct tpm_permanent_flags pf;
> > ret = read_permanent_flags((char *)&pf, sizeof(pf));
> > if (ret)
> >    return -1;
> > /* check if hardware physical presence is supported and asserted

*/
> > if (pf.flags[PERM_FLAG_IDX_PHYSICAL_PRESENCE_HW_ENABLE]) {
> >    int has_hw_pp;
> >    ret = has_hw_physical_presence(&pf, &has_hw_pp);
> >    if (verbose && !has_hw_pp)
> >        printf("Hardware physical presence is not 

asserted.\n\n");
> >    if (ret)
> >        return ret;
> >    if (has_hw_pp)
> >        return 0;
> >    if (!pf.flags[PERM_FLAG_IDX_PHYSICAL_PRESENCE_CMD_ENABLE]) {
> >        /* cannot enable phys. presence using command */
> >        if (verbose)
> >            printf("Error: Physical presence SW assertion is 
>
> disabled.\n\n");
> >        return -1;
> >    }
> > }
> > if (stcf.flags[STCLEAR_FLAG_IDX_PHYSICAL_PRESENCE_LOCK]) {
> >    /* physical presence cannot be changed anymore */
> >    if (verbose)
> >        printf("Error: Physical presence assertion is 

locked.\n\n");
> >    return -1;
> > }
> > ret = tpm_send_check_cmd(0, TPM_ORD_PhysicalPresence,
> >                             PhysicalPresence_CMD_ENABLE,
> >                             sizeof(PhysicalPresence_CMD_ENABLE),
>
> This seems to issue CMD_ENABLE even if CMD_ENABLE may have already
> been on - I'm leery of that.

I'll remove that.
> To summarize, what about this sequence during startup:
> PhysicalPresence_PRESENT
> if fail:
>    flags = read_permanent_flags()
>    if flags.HW_ENABLE:
>        return success
>    if !flags.CMD_ENABLE && !flags.LOCK:
>        PhysicalPresence_CMD_ENABLE
>        PhysicalPresence_PRESENT
>
> Then we don't have to issue PRESENT or CMD_ENABLE anywhere else in the
> code.

Ok.

Stefan