<tt><font size=2>"Kevin O'Connor" <kevin@koconnor.net>
wrote on 01/06/2016 03:20:56 PM:<br><br>> <br>> On Wed, Jan 06, 2016 at 01:15:55PM -0500, Stefan Berger wrote:<br>> > From: Stefan Berger <stefanb@linux.vnet.ibm.com><br>> > <br>> > Extend assert_physical_presence with checks for hardware physical
presence<br>> > support. If no hardware physical presence is asserted and the
SW assertion<br>> > is disable, -1 is returned.<br>> > <br>> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com><br>> > ---<br>> > src/std/tcg.h | 2 ++<br>> > src/tcgbios.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++<br>> +++++++++++--<br>> > 2 files changed, 61 insertions(+), 2 deletions(-)<br>> > <br>> > diff --git a/src/std/tcg.h b/src/std/tcg.h<br>> > index 9f7f021..00be533 100644<br>> > --- a/src/std/tcg.h<br>> > +++ b/src/std/tcg.h<br>> > @@ -70,6 +70,7 @@<br>> > <br>> > /* TPM command error codes */<br>> > #define TPM_INVALID_POSTINIT
0x26<br>> > +#define TPM_BAD_PRESENCE
0x2d<br>> > #define TPM_BAD_LOCALITY
0x3d<br>> > <br>> > /* TPM command tags */<br>> > @@ -285,6 +286,7 @@ enum permFlagsIndex {<br>> > PERM_FLAG_IDX_ALLOW_MAINTENANCE,<br>> > PERM_FLAG_IDX_PHYSICAL_PRESENCE_LIFETIME_LOCK,<br>> > PERM_FLAG_IDX_PHYSICAL_PRESENCE_HW_ENABLE,<br>> > + PERM_FLAG_IDX_PHYSICAL_PRESENCE_CMD_ENABLE,<br>> > };<br>> > <br>> > <br>> > diff --git a/src/tcgbios.c b/src/tcgbios.c<br>> > index d6a8495..6a6b6b0 100644<br>> > --- a/src/tcgbios.c<br>> > +++ b/src/tcgbios.c<br>> > @@ -446,7 +446,7 @@ err_exit:<br>> > return -1;<br>> > }<br>> > <br>> > -static u32<br>> > +static int<br>> > read_stclear_flags(char *buf, int buf_len)<br>> > {<br>> > memset(buf, 0, buf_len);<br>> > @@ -480,7 +480,32 @@ read_permanent_flags(char *buf, int buf_len)<br>> > return 0;<br>> > }<br>> > <br>> > -static u32<br>> > +static int<br>> > +has_hw_physical_presence(struct tpm_permanent_flags *pf, int
*has_hw_pp)<br>> > +{<br>> > + u32 ordinal;<br>> > +<br>> > + /* We cannot read hardware physical presence from
a flag;<br>> > + * it has to be inferred from the error code to
a command that<br>> > + * needs physical presence<br>> > + */<br>> > + if (pf->flags[PERM_FLAG_IDX_DISABLE])<br>> > + ordinal = TPM_ORD_PhysicalDisable;<br>> > + else<br>> > + ordinal = TPM_ORD_PhysicalEnable;<br>> > +<br>> > + int ret = tpm_send_cmd(0, ordinal, NULL, 0, TPM_DURATION_TYPE_SHORT);<br>> <br>> I'm leery of code that automatically issues a command that nominally<br>> alters non-volatile memory as I fear it could cause the hardware to<br>> wear out. So, I'd avoid doing this unless the above is definitely
not<br>> an issue.</font></tt><br><br><tt><font size=2>It's implementation-dependent what the TPM will do
once a permanent flag is set to the</font></tt><br><tt><font size=2>same value it already is. It may write it back into
NVRAM or not.</font></tt><br><tt><font size=2><br>> <br>> If you want the menu to also be usable on machines with hardware<br>> physical presence, then I'm fine with enabling the menu after just<br>> checking that HW_ENABLE is true.</font></tt><br><br><tt><font size=2>In this case we may encounter errors if hardware physical
presence is not actually set.</font></tt><br><tt><font size=2><br>> <br>> [...]<br>> > +static int<br>> > assert_physical_presence(int verbose)<br>> > {<br>> > struct tpm_stclear_flags stcf;<br>> > @@ -492,6 +517,38 @@ assert_physical_presence(int verbose)<br>> > /* physical presence already
asserted */<br>> > return 0;<br>> <br>> I don't think we need to read stclear flags here - I think it would
be<br>> simpler to just issue PhysicalPresence_PRESENT. If it succeeds
then<br>> we've successfully asserted physical presence, and if it fails then
go<br>> on to read permanent flags.</font></tt><br><br><tt><font size=2>Ok.</font></tt><br><tt><font size=2><br>> <br>> > + struct tpm_permanent_flags pf;<br>> > + ret = read_permanent_flags((char *)&pf, sizeof(pf));<br>> > + if (ret)<br>> > + return -1;<br>> > +<br>> > + /* check if hardware physical presence is supported
and asserted */<br>> > + if (pf.flags[PERM_FLAG_IDX_PHYSICAL_PRESENCE_HW_ENABLE])
{<br>> > + int has_hw_pp;<br>> > + ret = has_hw_physical_presence(&pf,
&has_hw_pp);<br>> > + if (verbose && !has_hw_pp)<br>> > + printf("Hardware
physical presence is not asserted.\n\n");<br>> > + if (ret)<br>> > + return ret;<br>> > +<br>> > + if (has_hw_pp)<br>> > + return 0;<br>> > +<br>> > + if (!pf.flags[PERM_FLAG_IDX_PHYSICAL_PRESENCE_CMD_ENABLE])
{<br>> > + /* cannot enable phys.
presence using command */<br>> > + if (verbose)<br>> > + printf("Error:
Physical presence SW assertion is <br>> disabled.\n\n");<br>> > + return -1;<br>> > + }<br>> > + }<br>> > +<br>> > + if (stcf.flags[STCLEAR_FLAG_IDX_PHYSICAL_PRESENCE_LOCK])
{<br>> > + /* physical presence cannot be changed
anymore */<br>> > + if (verbose)<br>> > + printf("Error:
Physical presence assertion is locked.\n\n");<br>> > + return -1;<br>> > + }<br>> > +<br>> > ret = tpm_send_check_cmd(0, TPM_ORD_PhysicalPresence,<br>> >
PhysicalPresence_CMD_ENABLE,<br>> >
sizeof(PhysicalPresence_CMD_ENABLE),<br>> <br>> This seems to issue CMD_ENABLE even if CMD_ENABLE may have already<br>> been on - I'm leery of that.</font></tt><br><br><tt><font size=2>I'll remove that.</font></tt><br><tt><font size=2><br>> <br>> To summarize, what about this sequence during startup:<br>> <br>> PhysicalPresence_PRESENT<br>> if fail:<br>> flags = read_permanent_flags()<br>> if flags.HW_ENABLE:<br>> return success<br>> if !flags.CMD_ENABLE && !flags.LOCK:<br>> PhysicalPresence_CMD_ENABLE<br>> PhysicalPresence_PRESENT<br>> <br>> Then we don't have to issue PRESENT or CMD_ENABLE anywhere else in
the<br>> code.</font></tt><br><br><tt><font size=2>Ok.</font></tt><br><br><tt><font size=2> Stefan</font></tt><br><BR>