[SeaBIOS] Re: [PATCH 2/2] tcgbios: Implement TPM 2.0 menu item to activate and deactivate PCR banks

[Stefan Berger]

On 1/29/19 4:24 PM, Kevin O'Connor wrote:
> On Tue, Jan 22, 2019 at 10:46:24AM -0500, Stefan Berger wrote:
> > Implement a TPM 2.0 menu item that allows a user to toggle the activation
> > of PCR banks of the TPM 2.0. After successful activation we shut down the
> > TPM 2.0 and reset the machine.
>
> Thanks.  In general, it looks fine to me.  Just for my understanding,
> could you give a high level overview of when/why a user would use this
> menu and what functionality they would miss if this menu were not
> available?

I'll add this to a v3: A TPM 2.0 may have multiple PCR banks, such as 
for SHA1, SHA256, SHA384, SHA512, and SM3-256. One or multiple of those 
banks may be active (by factory for example) and modifying the set of 
active PCR banks is only possible while in the firmware since it 
requires platform authorization. However, Per spec we generate a random 
password for the platform authorization before we activate the boot 
loader and throw that password away so that one cannot make any 
modifications that require this type of authorization while running an OS.

The only other method of doing this is via the Physical Presence 
Interface by posting a code (and parameter in this particular case) from 
the OS. But this requires the firmware to act on it upon reboot.
> I also have a few comments - see below.
>
> [...]
> > --- a/src/tcgbios.c
> > +++ b/src/tcgbios.c
>
> [...]
> > @@ -1780,6 +1926,13 @@ tpm20_process_cfg(tpm_ppi_code msgCode, int verbose)
> >               if (!ret)
> >                    ret = tpm20_clear();
> >               break;
> >    case TPM_PPI_OP_SET_PCR_BANKS:
> >        ret = tpm20_set_pcrbanks(pprm);
> >        if (!ret)
> >            ret = tpm_simple_cmd(0, TPM2_CC_Shutdown,
> >                                 2, TPM2_SU_CLEAR, TPM_DURATION_TYPE_SHORT);
> >        break;
> > }
> >
> > if (ret)
>
> [...]
> > static void
> >   tpm20_menu(void)
> >   {
> >       int scan_code;
> >       tpm_ppi_code msgCode;
> >
> > u32 pprm;
> >
> > u8 active_banks;
> >
> > int do_reset = 0;
> >
> > for (;;) {
> >      printf("1. Clear TPM\n");
> >    printf("2. Change active PCR banks\n");
> >
> >     printf("\nIf no change is desired or if this menu was reached by "
> >            "mistake, press ESC to\n"
> >
> > @@ -1988,11 +2204,20 @@ tpm20_menu(void)
> >           case 2:
> >               msgCode = TPM_PPI_OP_CLEAR;
> >               break;
> >    case 3:
> >        if (tpm20_menu_change_active_pcrbanks(&active_banks) < 0)
> >            continue;
> >        msgCode = TPM_PPI_OP_SET_PCR_BANKS;
> >        pprm = active_banks;
> >        do_reset = 1;
> >        break;
> >     default:
> >         continue;
> >     }
> >
> >    tpm20_process_cfg(msgCode, 0);
> >    tpm20_process_cfg(msgCode, pprm,  0);
> >    if (do_reset)
> >        reset();
> > }
>
> FWIW, I find the above confusing.  Could
> tpm20_menu_change_active_pcrbanks() just directly invoke
> tpm20_set_pcrbanks() et al when needed?

It certainly could. I can change this.
> > --- a/src/util.h
> > +++ b/src/util.h
> > @@ -38,6 +38,8 @@ struct usbdevice_s;
> >   int bootprio_find_usb(struct usbdevice_s *usbdev, int lun);
> >   int get_keystroke(int msec);
> >
> > +#define SCANCODE_A 0x1e
>
> The convention for util.h is to only have forward declarations for
> variables and functions - no macros, typedefs, or struct definitions.
> Also, you might want take a look at possibly using ascii_to_keycode().

Ok.

Stefan
> -Kevin