"Kevin O'Connor" <kevin@koconnor.net> 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 <stefanb@linux.vnet.ibm.com>
> >
> > 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 <stefanb@linux.vnet.ibm.com>
> > ---
> >  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