[SeaBIOS] [PATCH v9 2/6] Implementation of the TCG BIOS extensions

[Kevin O'Connor]

On Fri, Mar 20, 2015 at 02:00:37PM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefann at linux.vnet.ibm.com>
> 
> This patch implements the main part of the TCG BIOS extensions. It provides
> the following functionality:
> 
> - initialization of the TCPA ACPI table used for logging of measurements
> - initialization of the TPM by sending a sequence of commands to it
> - proper setup of the TPM before the BIOS hands over control to the bootloader
> - support for S3 resume; BIOS sends TPM_Startup(ST_STATE) to TPM
> - enable configuration of SeaBIOS to be built with TCGBIOS extensions
>   All TCG BIOS extensions are activated with CONFIG_TCGBIOS.
[...]
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -421,6 +421,13 @@ menu "BIOS interfaces"
>              modified by programs.  However, some old DOS high memory
>              managers may require the UMB region to be read-only.
>  
> +    config TCGBIOS
> +        select S3_RESUME
> +        bool "TPM support and TCG BIOS extensions"
> +        default y
> +        help
> +            Provide TPM support along with TCG BIOS extensions

I don't think TCGBIOS should select S3_RESUME.  If resume processing
is an absolute requirement then I think it should use "depends on".

[...]
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -19,6 +19,7 @@
>  #include "std/disk.h" // struct mbr_s
>  #include "string.h" // memset
>  #include "util.h" // irqtimer_calc
> +#include "tcgbios.h" // tpm_*
>  
>  
>  /****************************************************************
> @@ -475,6 +476,7 @@ interactive_bootmenu(void)
>  
>      printf("Select boot device:\n\n");
>      wait_threads();
> +    tpm_leave_bios();

Is this call really supposed to be in the boot menu?  The call wont
get invoked if the boot menu isn't selected.

> --- a/src/post.c
> +++ b/src/post.c
> @@ -28,6 +28,7 @@
>  #include "output.h" // dprintf
>  #include "string.h" // memset
>  #include "util.h" // kbd_init
> +#include "tcgbios.h" // tpm_*
>  
>  
>  /****************************************************************
> @@ -220,6 +221,10 @@ maininit(void)
>      if (threads_during_optionroms())
>          device_hardware_setup();
>  
> +    // Initialize tpm (after acpi tables were written)
> +    tpm_acpi_init();
> +    tpm_startup();

I think this should be just one call - such as "tpm_setup()", which in
turn calls tpm_xyz().

Also, functions with "_init" and "_setup" suffixes have specific
meaning in the bootstrap ordering sequence.  So tpm shouldn't export
functions with these suffixes unless they are invoked during the given
sequence.

[...]
> +u32
> +tpm_startup(void)
> +{
> +    u32 rc;
> +    u32 returnCode;
> +
> +    if (!CONFIG_TCGBIOS)
> +        return 0;
> +
> +    if (!has_working_tpm())
> +        return TCG_GENERAL_ERROR;
> +
> +    dprintf(DEBUG_tcg, "TCGBIOS: Starting with TPM_Startup(ST_CLEAR)\n");
> +    rc = build_and_send_cmd(TPM_ORD_Startup,
> +                            Startup_ST_CLEAR, sizeof(Startup_ST_CLEAR),
> +                            NULL, 10, &returnCode, TPM_DURATION_TYPE_SHORT);
> +
> +    dprintf(DEBUG_tcg, "Return code from TPM_Startup = 0x%08x\n",
> +            returnCode);
> +
> +#if CONFIG_COREBOOT
> +    /* with other firmware on the system the TPM may already have been
> +     * initialized
> +     */
> +    if (returnCode == TPM_INVALID_POSTINIT)
> +        returnCode = 0;
> +#endif

That '#if' should be turned into an 'if (...)'.

[...]
> +u32
> +tpm_s3_resume(void)
> +{
> +    u32 rc;
> +    u32 returnCode;
> +
> +    if (!CONFIG_TCGBIOS)
> +        return 0;
> +
> +    if (!has_working_tpm())
> +        return TCG_GENERAL_ERROR;
> +
> +    timer_setup();

Why is the timer reinitialized here?  There may be implications in
touching that hardware during an S3 resume.

> +
> +    dprintf(DEBUG_tcg, "TCGBIOS: Resuming with TPM_Startup(ST_STATE)\n");
> +
> +    rc = build_and_send_cmd(TPM_ORD_Startup,
> +                            Startup_ST_STATE, sizeof(Startup_ST_STATE),
> +                            NULL, 10, &returnCode, TPM_DURATION_TYPE_SHORT);
> +
> +    dprintf(DEBUG_tcg, "TCGBIOS: ReturnCode from TPM_Startup = 0x%08x\n",
> +            returnCode);
> +
> +    if (rc || returnCode)
> +        goto err_exit;
> +
> +    return 0;
> +
> +err_exit:
> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM malfunctioning (line %d).\n", __LINE__);
> +
> +    tpm_state.tpm_working = 0;
> +    if (rc)
> +        return rc;
> +    return TCG_TCG_COMMAND_ERROR;
> +}
> diff --git a/src/tcgbios.h b/src/tcgbios.h
> new file mode 100644
> index 0000000..7be89f8
> --- /dev/null
> +++ b/src/tcgbios.h
> @@ -0,0 +1,388 @@
> +#ifndef TCGBIOS_H
> +#define TCGBIOS_H
> +
> +#include "types.h"
> +#include "bregs.h" /* struct bregs */

Why is "bregs.h" needed in the header - can it be included in the
individual C files that need it?


As before, the above look minor to me and I think they could be
addressed after merging.  Based on your previous mail, I'm focusing
review comments on patches 1 and 2 right now.

-Kevin