On Mon, Dec 21, 2015 at 11:50:07AM -0500, Stefan Berger wrote:
> "Kevin O'Connor" <kevin(a)koconnor.net> wrote on 12/17/2015 05:22:56 PM:
>
> >
> > On Mon, Nov 30, 2015 at 11:32:05AM +0000, Wim Vervoorn wrote:
> > > Hello,
> > >
> > > I noticed that a lot of work is going on for the TPM support in
> SeaBIOS.
> > >
> > > All of this work is TPM 1.2 based. I was wondering if there are any
> > > plans to support …
[View More]TPM 2.0 in the future.
> >
> > I'm not aware of any plans.
>
> We're working on it...
>
>
> So maybe you have some comments to the following:
>
> There will be a patch for probing the TPM TIS hardware interface for
> whether there's a TPM 1.2 or a TPM 2.
> We then have a patch for prefixing all TPM 1.2 functions with tpm12_ and
> then introduce functions like these ones here:
>
> static ... tpm12_foo() { ... }
> static ... tpm2_foo() { ... }
>
> tpm_foo()
> {
> [...]
>
> switch (tpmversion) {
> case TPM_VERSION_1_2:
> tpm12_foo()
> break;
> case TPM_VERSION_2:
> tpm2_foo();
> break;
> }
>
> [...]
> }
Is the difference between 1.2 and 2.0 so large that the above is
needed?
-Kevin
[View Less]
Hi Stefan,
I cleaned up the additional patches I mentioned yesterday.
Some time back, you posted a series of patches that removed the use of
TCG 16bit BIOS structs from internal functions. Many of those
functions were still using return codes from the spec though. I found
using these return codes in internal functions to be annoying because
they conflict with the TIS return codes. This series mostly just
separates out the return codes so that only the TCG 16bit BIOS
functions deal with the …
[View More]TCG 16bit BIOS return codes.
This series is on top of patches 1-9 of the previous series (it
replaces patch 10 of the previous series). Both series are also
available at:
https://github.com/KevinOConnor/seabios/tree/testing
I'm not currently planning any other TPM changes after these.
-Kevin
Kevin O'Connor (8):
tpm: Don't return a status from external bios measurement functions
tpm: No need to check the return status of measurements
tpm: Don't call tpm_set_failure() from tpm_log_extend_event()
tpm: Don't use 16bit BIOS return codes in build_and_send_cmd()
tpm: Don't use 16bit BIOS return codes in tpm_log_event()
tpm: Don't use 16bit BIOS return codes in tpmhw_* functions
tpm: Don't use 16bit BIOS return codes in TPM menu functions
tpm: Replace build_and_send_cmd with tpm_send_cmd and
tpm_send_check_cmd
src/hw/tpm_drivers.c | 20 +-
src/hw/tpm_drivers.h | 2 +-
src/tcgbios.c | 718 +++++++++++++++++++--------------------------------
src/tcgbios.h | 8 +-
4 files changed, 287 insertions(+), 461 deletions(-)
--
2.5.0
[View Less]
The following series involves some code reorganization in the TPM code
that I found useful in understanding the code.
Patches 3-5 simplify the hardware interface by only exporting three
commands to the underlying TIS hardware (tpmhw_probe, tpmhw_transmit,
tpmhw_set_timeouts).
Patches 8-10 simplify the parameters to the build_and_send_cmd()
function.
The remaining patches are mostly just code reorg.
I have only compile tested these changes.
-Kevin
Kevin O'Connor (10):
tpm: Add banner …
[View More]separating the TCG bios interface code from TCG menu
code
tpm: Avoid macro expansion of tpm request / response structs
tpm: Simplify hardware probe and detection checks
tpm: Add wrapper function tpm_set_timeouts()
tpm: Move TPM hardware functions from tcgbios.c to hw/tpm_drivers.c
tpm: Rework TPM interface shutdown support
tpm: Simplify tcpa probe
tpm: Introduce tpm_get_capability() helper function
tpm: Eliminate response buffer parameter from build_and_send_cmd()
tpm: Return returnCode from build_and_send_cmd() instead of via
pointer param
src/hw/tpm_drivers.c | 84 ++++++
src/hw/tpm_drivers.h | 28 +-
src/std/tcg.h | 55 ++--
src/tcgbios.c | 704 +++++++++++++++------------------------------------
4 files changed, 317 insertions(+), 554 deletions(-)
--
2.5.0
[View Less]
On Thu, Dec 24, 2015 at 03:45:03PM -0800, Alex Gagniuc wrote:
> When SeaBIOS is run on an FMAP-based CBFS (AKA, without a CBFS master
> header, it crashes). The problem is in coreboot_cbfs_init. A
> disassembly with -Mintel shows the following:
>
> 00000000 <coreboot_cbfs_init>:
> ...
> 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc
> d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
>
> > 7: 8b 35 fc ff ff ff mov …
[View More]esi,DWORD PTR ds:0xfffffffc
>
> Move the contents of memory address 0xfffffffc to esi. Since there's
> no legacy CBFS pointer, the region is all 1's. esi now contains
> 0xffffffff
>
> > d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
>
> 32-bit dereference esi (0xffffffff) -- causes general protection
> fault. The dereference is not because of unaligned access, but because
> the access goes over the 32-bit address space in flat mode. The way
> around this iss to sanitize the value at [0xffffffff] before
> dereferencing it. A sane check is:
>
> if(address_of_hdr > (0xfffffff0 - sizeof(*hdr)))
> fail();
> This is sane check because because the x86 reset vector is resides at
> 0xfffffff0 (there are cases where this isn't true, but that's besides
> the point here).
Thanks. Can you put together a patch, or do you want me to?
> There's also an unrelated problem with the C code that generates this assembly:
>
> > struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4);
>
> With CONFIG_CBFS_LOCATION = 0, this places a negative value (-4) in an
> unsigned type (pointer). The behavior of this is undefined according
> to the C standard.
Are you suggesting that an (unsigned) cast should be added? I doubt
it would matter.
-Kevin
[View Less]
The scsi_is_ready() function may be called from a thread, and it is
not valid to call printf() from a thread. Convert printf() to
dprintf() to avoid this possibility.
This does mean that cdrom detection (from cdrom_boot() ) may not give
notification of slow cdrom drives to a user. However, the extra
medium detection time is unlikely to be large anyway.
Reported-by: Tobias Diedrich <tobiasdiedrich(a)gmail.com>
Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net>
---
src/hw/…
[View More]blockcmd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c
index e20e3fc..0725b46 100644
--- a/src/hw/blockcmd.c
+++ b/src/hw/blockcmd.c
@@ -168,7 +168,7 @@ scsi_is_ready(struct disk_op_s *op)
if (sense.asc == 0x04 && sense.ascq == 0x01 && !in_progress) {
/* IN PROGRESS OF BECOMING READY */
- printf("Waiting for device to detect medium... ");
+ dprintf(1, "Waiting for device to detect medium... ");
/* Allow 30 seconds more */
end = timer_calc(30000);
in_progress = 1;
--
2.5.0
[View Less]
The NMI could occur when already on the extra stack, which would
corrupt it. Always use the current stack on an NMI to avoid this.
Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net>
---
src/romlayout.S | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/romlayout.S b/src/romlayout.S
index fedadfe..53cc0f5 100644
--- a/src/romlayout.S
+++ b/src/romlayout.S
@@ -548,7 +548,10 @@ entry_post:
ENTRY_INTO32 _cfunc32flat_handle_post // Normal entry point
…
[View More] ORG 0xe2c3
- IRQ_ENTRY 02
+ .global entry_02
+entry_02:
+ ENTRY handle_02 // NMI handler does not switch onto extra stack
+ iretw
ORG 0xe3fe
.global entry_13_official
--
2.5.0
[View Less]
When SeaBIOS is run on an FMAP-based CBFS (AKA, without a CBFS master
header, it crashes). The problem is in coreboot_cbfs_init. A
disassembly with -Mintel shows the following:
00000000 <coreboot_cbfs_init>:
...
7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc
d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
> 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc
Move the contents of memory address 0xfffffffc to esi. Since there's
no legacy CBFS …
[View More]pointer, the region is all 1's. esi now contains
0xffffffff
> d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f
32-bit dereference esi (0xffffffff) -- causes general protection
fault. The dereference is not because of unaligned access, but because
the access goes over the 32-bit address space in flat mode. The way
around this iss to sanitize the value at [0xffffffff] before
dereferencing it. A sane check is:
if(address_of_hdr > (0xfffffff0 - sizeof(*hdr)))
fail();
This is sane check because because the x86 reset vector is resides at
0xfffffff0 (there are cases where this isn't true, but that's besides
the point here).
There's also an unrelated problem with the C code that generates this assembly:
> struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4);
With CONFIG_CBFS_LOCATION = 0, this places a negative value (-4) in an
unsigned type (pointer). The behavior of this is undefined according
to the C standard.
Hope this helps,
Alex
[View Less]
On Wed, Dec 23, 2015 at 06:40:12AM +0000, Gonglei (Arei) wrote:
> > From: Kevin O'Connor [mailto:kevin@koconnor.net]
> > On Tue, Dec 22, 2015 at 02:14:12AM +0000, Gonglei (Arei) wrote:
> > > Sorry, it doesn't work. What's worse is we cannot stop SeaBIOS stuck by
> > > Setting "CONFIG_ENTRY_EXTRASTACK=n" after applying this patch.
> >
> > Oops, can you try with the patch below instead?
> >
>
> It works now. Thanks!
>
> But do we …
[View More]need to check other possible situations
> that maybe cause *extra stack* broken or overridden?
I believe the issue is that an NMI could occur while SeaBIOS is
already using its extra stack. The code is not prepared to switch
into the extra stack while already on the extra stack. SeaBIOS is
careful to always disable IRQs while running C code to prevent this
issue, but disabling normal IRQs does not disable NMIs. So, I believe
this issue is specific to the nature of NMIs.
-Kevin
[View Less]