[SeaBIOS] [PATCH 1/3] Add 'measurement' code to the BIOS

[Kevin O'Connor]

On Fri, May 08, 2015 at 01:45:46PM -0400, Stefan Berger wrote:
> This patch adds invocations of functions that measure various parts of the
> code and data through various parts of the BIOS code. It follows TCG
> specifications on what needs to be measured. It also adds the implementation
> of the called functions.
> 
> Reference for what needs to be measured can be found in section 3.2.2++ in
> 
> http://www.trustedcomputinggroup.org/resources/pc_client_work_group_specific_implementation_specification_for_conventional_bios_specification_version_12
> 
> 
> The first measurements are done once the ACPI tables have been initialized.

Thanks - I have some questions and comments below.

> Once booted into Linux, the current measurements produce the following logs
> which can be found in /sys/kernel/security/tpm0/ascii_bios_measurements.
> The below log also shows measurements from trusted grub.
> 
>  1 3fb240d2a04085a4e84f81e4398e070ed5a18163 06 [SMBIOS]
>  2 cc812353fc277c1fab99e0b721752a1392984566 06 [Option ROM]
>  2 9dbd87163112e5670378abe4510491259a61f411 05 [Start Option ROM Scan]
>  2 6f74e357331b8dee11bbad85f27bc66cb873106c 06 [Option ROM]
>  2 5626eb7ac05c7231e46d7461e7d3839b03ae9fad 06 [Option ROM]
>  4 c1e25c3f6b0dc78d57296aa2870ca6f782ccf80f 05 [Calling INT 19h]
>  0 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  1 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  2 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  3 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  4 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  5 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  6 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  7 d9be6524a5f5047db5866813acf3277892a7a30a 04 []
>  4 8cf2fe6c87d4d0b2998a43da630292e6d85ee8b6 05 [Booting BCV device 80h (HDD)]
>  4 5dff94459a3e2d13a433ef94afdc306144565bf7 0d [IPL]
>  5 d1b33afde65ad47502332af957c60f20c84c1edc 0e [IPL Partition Data]
>  4 487ce764b527ccad17f1d04243d0136fa981e6c4 0d [IPL]
>  4 91d285e4dead566324c8938a3cc75803f462d9a1 0d [IPL]
>  4 8ba79ac98bb491524fef29defc724daaf6263d35 0d [IPL]
>  4 c591c15b82e4ff30e7383a4ff1ef3b41b38521ac 06 []
>  4 8cdc27ec545eda33fbba1e8b8dae4da5c7206972 04 [Grub Event Separator]
>  5 8cdc27ec545eda33fbba1e8b8dae4da5c7206972 04 [Grub Event Separator]
>  5 e8673b9e14b02dc12d8ccfd0176bca7a3de7fc3c 0e [IPL Partition Data]
>  5 0163e375a0af7525c5dac1a8e74b277359e40d1d 1105 []
>  8 4be30f67c3d48ab7f04d9c0fd07f06d4c68379be 1205 []
>  8 54c83965978de9708d026016ecb0e70660e04388 1305 []
>  5 2431ed60130faeaf3a045f21963f71cacd46a029 04 [OS Event Separator]
>  8 2431ed60130faeaf3a045f21963f71cacd46a029 04 [OS Event Separator]
>  8 f3973cae05d6e2055062119d6e6e1e077b7df876 1005 []

Which of these measurements are required to be performed and which are
optional?

> --- 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_*
>  
>  
>  /****************************************************************
> @@ -628,6 +629,10 @@ boot_disk(u8 bootdrv, int checksig)
>          }
>      }
>  
> +    /* specs: 8.2.3 steps 4 and 5 */
> +    tpm_add_bootdevice_ipl(0, bootdrv,
> +                           IPL_BCV, MAKE_FLATPTR(bootseg, 0), 512);

I think the comments with "specs:" are confusing as they are
references to the tpm specs instead of the BIOS boot specifications.
I think the "spec" comments should be moved into the tcg code.

Also, the parameters to tpm_add_bootdevice_ipl() seem complex - I
wonder if a couple of additional functions (eg, tpm_add_bcv(),
tpm_add_cdrom(), tpm_add_cdrom_catalog() ) would make the calling code
simpler.

[...]
> @@ -651,6 +656,11 @@ boot_cdrom(struct drive_s *drive_g)
>  
>      u8 bootdrv = CDEmu.emulated_drive;
>      u16 bootseg = CDEmu.load_segment;
> +
> +    /* specs: 8.2.5.6 */
> +    tpm_add_bootdevice_ipl(1, bootdrv,
> +                           IPL_EL_TORITO_1, MAKE_FLATPTR(bootseg, 0), 512);

This measurement seems redundant with the measurement already taken in
cdrom.c.

[...]
> @@ -733,6 +743,8 @@ do_boot(int seq_nr)
>          break;
>      }
>  
> +    tpm_returned_via_int18h();

I don't understand the name returned_via_int18h here as we're calling
int18 in this location, not returning from it.

[...]
> --- a/src/optionroms.c
> +++ b/src/optionroms.c
> @@ -19,6 +19,7 @@
>  #include "std/pnpbios.h" // PNP_SIGNATURE
>  #include "string.h" // memset
>  #include "util.h" // get_pnp_offset
> +#include "tcgbios.h" // tpm_*
>  
>  static int EnforceChecksum, S3ResumeVga, RunPCIroms;
>  
> @@ -80,6 +81,7 @@ is_valid_rom(struct rom_header *rom)
>          if (EnforceChecksum)
>              return 0;
>      }
> +    tpm_option_rom(rom, len);

Taking a measurement from is_valid_rom() doesn't seem correct (as
is_valid_rom can be called multiple times for the same rom).  Wouldn't
the measurement make more sense just before calling the rom (ie, in
callrom() ) or just after copying it to memory?

[...]
> @@ -354,6 +356,8 @@ optionrom_setup(void)
>      memset(sources, 0, sizeof(sources));
>      u32 post_vga = rom_get_last();
>  
> +    tpm_start_option_rom_scan();

I think this should be folded into tpm_start().

[...]
> --- a/src/post.c
> +++ b/src/post.c
> @@ -197,6 +197,9 @@ prepareboot(void)
>  void VISIBLE32FLAT
>  startBoot(void)
>  {
> +    tpm_calling_int19h();
> +    tpm_add_event_separators();

These calls shouldn't be done from startBoot() - the VISIBLE32FLAT
flag does something special to note the end of "init" code sections.
I think these calls should be folded into tpm_leave_bios().

[...]
> @@ -223,6 +226,7 @@ maininit(void)
>  
>      // Initialize TPM
>      tpm_start();
> +    tpm_smbios_measure();

This call should be folded into tpm_start().

-Kevin