Re: [Qemu-devel] [PATCH V1 6/8] Add measurement code to the BIOS

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 04/04/2011 12:57 AM, Kevin O'Connor wrote:
> On Wed, Mar 30, 2011 at 01:55:40PM -0400, Stefan Berger wrote:
>> This patch adds invocactions 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.
> [...]
>>   void VISIBLE32FLAT
>>   startBoot(void)
>>   {
>> +    tcpa_calling_int19h();
>> +    tcpa_add_event_separators();
> Why add two functions here, instead of just one function that does
> both actions?
I kept them separate since they're two different 'events', which would 
allow to put another call in between, if another event was to be 
measured - currently there is none. I'll combine the calls.
> [...]
>>       // Initialize tpm (after acpi tables were written)
>>       tcpa_acpi_init();
>>       tcpa_startup();
>> +    tcpa_measure_post((void *)0xE0000, (void *)0xFFFFF);
>> +    tcpa_smbios_measure();
> Same here.  Also, I'm not sure I understand why you're measuring
> 0xE0000-0xFFFFF - if the intent is to measure the code, then it's a
> bit more complicated as the init code has already been relocated by
> this point.
Basically, I wanted to start the measuring as early as possible, ideally 
measuring layers or functions sequentially, which turns out to be quite 
'tricky'. So this was the brute force compromise. I'll drop it and will 
start with the smbios measurement for now.
> [...]
>> +    tcpa_option_rom(FLATPTR_TO_SEG(rom), len);
> You're better off just defining tcpa_option_rom() to take a regular
> pointer.
Done.
> [...]
>> +    /* specs: 8.2.3 step 5 and 8.2.5.6, measure El Torito boot catalog */
>> +    /* measure 2048 bytes (one sector) */
>> +    tcpa_add_bootdevice(1, 0);
>> +    tcpa_ipl(IPL_EL_TORITO_2, GET_SEG(SS), (u32)buffer, 2048);
> Same here for tcpa_ipl().  GET_SEG() always returns 0 in 32bit mode
> and this code is only run in 32bit mode.
>
Done.
> [...]
>> +static struct smbios_entry_point *
>> +find_smbios_entry_point(void)
>> +{
> The smbios table is built in smbios.c:smbios_entry_point_init() -
> instead of scanning for the table, just record where the table is in a
> global variable.
Done.
> [...]
>> +u32
>> +tcpa_smbios_measure(void)
>> +{
>> +    u32 rc;
>> +    struct pcctes pcctes = {
>> +        .eventid = 1, /* 10.4.2.3.1 */
>> +        .eventdatasize = SHA1_BUFSIZE,
>> +    };
>> +    struct smbios_entry_point *sep = find_smbios_entry_point();
>> +
>> +    if (!has_working_tpm())
>> +        return 0;
> BTW - SeaBIOS uses C99, so it's fine to move "if (!has_working_tpm())"
> above the variable declarations.
>
I am moving all these checks above the var decls.

   Stefan
> -Kevin