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

[Kevin O'Connor]

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.

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.

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

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.

-Kevin