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

[Stefan Berger]

On 05/21/2015 01:40 PM, Kevin O'Connor wrote:
> 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?

The above list shows some measurements from trusted grub, e.g. those 
after the first IPL Partition Data are from trusted grub.
The spec doesn't say which ones are optional but we are more on the 
'light' side in terms of measurements.

>
>> --- 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.

Will do. Maybe even remove them or reference via title. The numbers 
changed with different versions of the specs.

>
> 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.

Will look into it.

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


I did this assuming that we will eventually have to return from int18.


>
> [...]
>> --- 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().


Option ROMs will always be scanned, so ok, I'll also move it into 
tpm_start(). The intention previously was to do the measurements at the 
location where corresponding 'action' happens, but I think you don't 
want the code to be filled with these calls.


>
> [...]
>> --- 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().


It works the way it is. I can move it, though.

>
> [...]
>> @@ -223,6 +226,7 @@ maininit(void)
>>   
>>       // Initialize TPM
>>       tpm_start();
>> +    tpm_smbios_measure();
> This call should be folded into tpm_start().

Definitely.

    Stefan

>
> -Kevin
>