From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41165 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q6mnc-0004TN-Oj for qemu-devel@nongnu.org; Mon, 04 Apr 2011 12:38:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q6mnb-0002ME-Di for qemu-devel@nongnu.org; Mon, 04 Apr 2011 12:38:44 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:48825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q6mnb-0002M1-BB for qemu-devel@nongnu.org; Mon, 04 Apr 2011 12:38:43 -0400 Received: from d01dlp01.pok.ibm.com (d01dlp01.pok.ibm.com [9.56.224.56]) by e9.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p34GBAcj024400 for ; Mon, 4 Apr 2011 12:11:10 -0400 Received: from d01relay07.pok.ibm.com (d01relay07.pok.ibm.com [9.56.227.147]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id EAC8638C803C for ; Mon, 4 Apr 2011 12:38:33 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay07.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p34GcWof1560624 for ; Mon, 4 Apr 2011 12:38:32 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p34GcWXc029379 for ; Mon, 4 Apr 2011 13:38:32 -0300 Message-ID: <4D99F407.5010506@linux.vnet.ibm.com> Date: Mon, 04 Apr 2011 12:38:31 -0400 From: Stefan Berger MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH V1 6/8] Add measurement code to the BIOS References: <20110330175534.302129463@linux.vnet.ibm.com> <20110330175600.001739348@linux.vnet.ibm.com> <20110404045735.GD13528@morn.localdomain> In-Reply-To: <20110404045735.GD13528@morn.localdomain> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: seabios@seabios.org, qemu-devel@nongnu.org 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