Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iommu.c: rework initialization function #17

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

krystian-hebel
Copy link
Member

Test for INVALIDATE_ALL completion is now done inside the initialization function, instead of main.c. Function names were changed to better describe what the function does. Comment describing window in anti-DMA security was updated with new finds.

Two bugs were fixed:

  • EventLogInt wasn't properly cleared (it is write-1-to-clear bit)
  • CmdBufEn was set on transition to the kernel, which didn't clear it before setting up head/tail pointers, which in turn lead to hang (it is listed as undefined behavior in IOMMU specification)

iommu.c Show resolved Hide resolved
iommu.c Outdated Show resolved Hide resolved
iommu.c Show resolved Hide resolved
@krystian-hebel
Copy link
Member Author

krystian-hebel commented Apr 11, 2024

SNAFU. Binary built locally from this code (on old Ubuntu, will check newer Fedora) works, while binary built by CI reports wrong self-measurement hash. Other hash values seem to be valid (PCR replay gives current value after fixing just the first entry), so the problem most likely lies in _end_of_measured. I think this might be caused by -R '.note.*'.

EDIT: shouldn't be, _end_of_measured has the same address as base of .note.gnu.property:

[khebel@3M05 secure-kernel-loader]$ objdump skl -h

skl:     file format elf64-x86-64

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         000029a0  0000000000000000  0000000000000000  00001000  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       00000270  00000000000029a0  00000000000029a0  000039a0  2**5
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  2 .note.gnu.property 00000030  0000000000002c10  0000000000002c10  00003c10  2**3
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  3 .bss          0000a4d4  0000000000003000  0000000000003000  00004000  2**12
                  ALLOC
  4 .comment      0000002e  0000000000000000  0000000000000000  00003c40  2**0
                  CONTENTS, READONLY
  ( ... more debug sections ... )
[khebel@3M05 secure-kernel-loader]$ objdump skl -t | grep _end_of_measured
0000000000002c10 g       .rodata	0000000000000000 .hidden _end_of_measured

@krystian-hebel
Copy link
Member Author

Local build on Fedora also doesn't give proper hash...

@krystian-hebel
Copy link
Member Author

Something changed in gcc-11, it fails to produce valid SHA1 measurements: #19

Base automatically changed from slrt to shrink_measured_size April 16, 2024 10:28
Test for INVALIDATE_ALL completion is now done inside the initialization
function, instead of main.c. Function names were changed to better
describe what the function does. Comment describing window in anti-DMA
security was updated with new finds.

Some bugs were fixed:
- EventLogInt wasn't properly cleared (it is write-1-to-clear bit)
- CmdBufEn was set on transition to the kernel, which didn't clear it
  before setting up head/tail pointers, which in turn lead to hang
  (it is listed as undefined behavior in IOMMU specification)
- Command buffer tail/head pointer registers were incorrectly masked

Signed-off-by: Krystian Hebel <[email protected]>
Release versions of SKL hanged in tis_send(), while versions with serial
output worked, most likely thanks to increased delay between DRTM
sequence and next TPM command. Apparently, some TPMs don't properly
handle setting STS.commandReady when all of the following conditions are
met:

- TPM has recently finished DRTM sequence (internal work may still be
  happening at that point, there is no way to be sure),
- TPM just transitioned to Idle state (e.g. by changing locality),
- STS.commandReady is written periodically before TPM reports it is in
  Ready state.

When all of the above applies, STS.commandReady is always read as 0,
as if the TPM restarted transition from Idle to Ready each time it is
asked to do so. To work around this, set this bit once and keep checking
in a loop until TIMEOUT_B (2 seconds). Well behaving TPM must be able to
enter Ready state before that time, if it doesn't, error is returned.

Signed-off-by: Krystian Hebel <[email protected]>
This enables SHA tests to check if different compilers produce the same
results.

Definitions in string.h were moved outside of __STDC_HOSTED__ to avoid
compiler warnings (promoted to errors because of -Werror) due to
incompatible implicit declaration of built-in functions.

Signed-off-by: Krystian Hebel <[email protected]>
-fstrict-aliasing is enabled by default when using optimization levels
higher than 1, including -Os. With that option, compiler may assume
that object of one type never resides at the same address as object of
a different type. Both sha1_final() and sha256_final() used to write
message length by casting a pointer to buffer into a pointer to u64,
while surrounding code operated on the buffer directly.

The problem manifests in GCC 11 and later versions.

The commit fixes this issue and another UB caused by unaligned access
at the beginning of transformation step by using memcpy() in both cases.

Signed-off-by: Krystian Hebel <[email protected]>
Signed-off-by: Sergii Dmytruk <[email protected]>
@macpijan macpijan merged commit c00c554 into shrink_measured_size Apr 17, 2024
127 of 131 checks passed
@macpijan macpijan deleted the iommu_updates branch April 17, 2024 17:47
@macpijan macpijan mentioned this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants