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.

Sign up for GitHub

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

Jump to bottom

Test mok mirror #394

Merged
merged 23 commits into from
Sep 7, 2021
Merged

Test mok mirror #394

merged 23 commits into from
Sep 7, 2021

Conversation

vathpela
Copy link
Contributor

@vathpela vathpela commented Jul 26, 2021 •
edited

This adds test cases for #387, which is also the second commit in the series.

Additionally, a memory leak from retrieving the same variable multiple times is fixed and a test for that problem is also added.

@vathpela vathpela force-pushed the test-mok-mirror branch 2 times, most recently from e30f9e6 to c52dc67 Compare July 27, 2021 14:39
@vathpela vathpela force-pushed the test-mok-mirror branch 2 times, most recently from ca116c7 to d8fc6c1 Compare August 4, 2021 21:48
@vathpela vathpela mentioned this pull request Aug 4, 2021
Closed
@vathpela vathpela force-pushed the test-mok-mirror branch 11 times, most recently from ae5af10 to 9a979ab Compare August 10, 2021 17:34
@vathpela vathpela force-pushed the test-mok-mirror branch 5 times, most recently from b5aaa38 to 50cc291 Compare August 17, 2021 13:51
@vathpela vathpela marked this pull request as ready for review August 17, 2021 13:58
frozencemetery
frozencemetery reviewed Sep 7, 2021
View reviewed changes
Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see anything obviously wrong here. One comment inline, and some feedback on commit messages:

  • A few commit messages are missing DCO.
  • Several commit messages have periods at the end of their subjects (unsure if this is important to you; feel free to ignore if it's not).
  • "make gcc ignore its mistake even harder" captures the frustration but not what the issue is :)

tpm.c Outdated
FreePool(vr->VariableName);
if (vr->VendorGuid)
FreePool(vr->VendorGuid);
if (vr->Data && vr->Size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data allocation is not conditional on Size != 0 in tpm_record_data_measurement(), so checking it here seems unnecessary, though probably harmless. (And if the allocation returned NULL, measuredcount would not include the entry.)

vathpela and others added 4 commits September 7, 2021 16:48
@vathpela
Make CopyMem() work with EFI's declaration
e3e6e72 
EFI_BOOT_SERVICES includes CopyMem() and SetMem() functions which are
marked EFIAPI, and in the case of CopyMem() does not mark the source
argument as CONST.

This patch makes all our invocations work with that, so (once gnu-efi's
implementation is fixed to match) we can use the existing implementation
as the implementation in a mock EFI_BOOT_SERVICES.

Signed-off-by: Peter Jones <pjones@redhat.com>
@lcp @vathpela
mok: delete the existing RT variables only when only_first=TRUE
34460fe 
For the firmware without the variable writing issues, MOK variables are
mirrored when only_first=TRUE. However, LibDeleteVariable() was called
in maybe_mirror_one_mok_variable() when only_first=FALSE, and this
could delete MOK variables that were just mirrored in the first round.

This bug was hidden since LibDeleteVariable() deletes BS+RT+NV variables
while we mirror MOK variables as BS+RT, and the firmware refused to
delete the mirrored MOK variable due to mismatching attributes. However,
some firmwares, such as VMWare, didn't enforce the attribute check and
just deleted the variables with matched name and GUID. In such system,
MokListRT was always removed before it reached OS.

Fixes: rhboot#386

Signed-off-by: Gary Lin <glin@suse.com>
@vathpela
Update gnu-efi
d0f98c5 
This updates gnu-efi to the shim-15.5 tag, with the following minor fixes:

5bd501b7e00 - Add missing EFI_VARIABLE_... definition.
9490cfe5bf2 - Check if we're using clang *before* we check the gcc version
3d2ba813d04 - Use CFLAGS_LTO and CFLAGS_GCOV variables during build
2186b121724 - Add some missing definitions for system table revisions
acc5371207f - Make CopyMem() and SetMem() be EFIAPI

With the exception of acc5371207f, all of these are needed for patches
further along in this patch series.  acc5371207f goes with the prior
patch in this series ("Make CopyMem() work with EFI's declaration."),
but they have to be applied in this order to keep from breaking the
build.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
Add some more stuff to .gitignore
681a6c9 
This adds compile_commands.json (used by https://github.com/neoclide/coc.nvim)
and clangd's .cache/ directory to .gitignore.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
Ignore a gcc issue in test-str.c
13cb202 
In 33db42d, we switched a gcc bounds checking error in test-str.c
to be a warning, due to it being obviously wrong and only occurring with
some GCC optimization flags.  For more details see the comment in
test-str.c .

It continues to be an issue, and the warning makes the -fanalyzer and
similar tools even harder to read, so it's better to just turn it off.
This is only in the test data, so if something really goes wrong we
should see it in failures or in valgrind when running tests.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
More minor improvements to support for COMPILER=clang
4594297 
A couple of places snuck in where building with COMPILER=clang didn't
work right; this makes them work again.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
cleanup: always use BS and RT, not gBS and gRT
da41fa6 
This just makes one less thing we have to make sure is the same between
the test harnesses and the runtime code.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: clean up temp data after running tests
e401272 
This makes sure we clean up the builds that aren't for the EFI
environment after we build and run the unit tests.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: make the CompareGuid() comparison size be less stupid
42246db 
5f08e67 introduced a CompareGuid() call in the unit test harness,
but unfortunately it has a typo and thus only ever compares the first
pointer-sized word of the guid.  With 4-GUIDs, this will usually produce
the correct results; with 1-GUIDs it often won't.

A second issue is that the memcmp() implementation of CompareGuid()
produces a different sort order than comparing field-by-field, and also
a different sort order than comparing the string representation.  This
is often not a problem (edk2, for example, never compares anything
except equality of two GUIDs), but when writing test cases it is
extremely helpful to be able to look at a list that is sorted in an
intuitive order.

This patch introduces a guidcmp() function in the test suite, which
compares the binary data in the same order that comparing the two GUIDs'
string representations would.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
mok: move the mok_state_variables definitions to their own header
7479ee2 
This lets us access the definitions for this structure, and the data
being used at runtime, from unit tests.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
shim/mm/fb: move global state to its own source file
dcc968c 
This moves the globals from shim.c (and lib/console.c) into their own
file, to make it so that unit tests can more easily link against code
that uses that state.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tpm: free measureddata when SHIM_UNIT_TEST is set
4ba12de 
Keep from cluttering up valgrind with allocations that aren't part of
the tested info (yet).

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: Disable some logging when SHIM_UNIT_TEST is enabled
4a74db1 
None of this should ever actually get called when we're running any of
the unit tests we've got, but some older compilers (i.e. Centos 7's gcc)
fail to remove some of the intermediate functions, and that causes a
link error with the functions they call.

This patch makes the top level call go away as well, so that the
intermediates never have linkage to the underlying implementation
functions.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: link all tests against libefivar
35b9fc4 
When writing new tests, if we get to the point where we have to use
libefivar for something, it's very common that I accidentally link it in
twice.  When that happens, I typically spend an unfortunate amount of
time staring at FLTO's mangled names before I figure out what I've done
wrong.

This patch makes all the tests link against libefivar, thereby
avoiding the issue.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: Add a 'test-coverage' make target for gcov
156df09 
This adds a make target that builds the tests with gcov so we can
identify coverage gaps in the test suite.

It also makes a special test-lto invocation, so that a developer can run
these tests with the somewhat different optimization results LTO will
have.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: add an efi error decoder
52b6305 
Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: add some slightly better EFIAPI error mocks
15339ea 
This adds more mock functions that just return various EFI error codes
in the EFIAPI ABI.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: add a mock implementation of {Get,Set}Variable and tests for it
71cc981 
Some tests will need variables, and so we need a mock implementation of
the various calls relating to them.

This patch adds implementations for the EFI Runtime Services calls
GetVariable(), SetVariable(), GetNextVariableName(), and
QueryVariableInfo().  Additionally, it enforces tunable limits on
storage for variables, and (with only a little work) the limits can be
different for SetVariable() vs what is returned by QueryVariableInfo().
That is, it can lie to you like real systems do.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: model different behaviors for deleting variables
fa4fd86 
Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: add pre and post hooks to the mock variable store
ea23585 
For testing of the mok mirroring behavior, we have to be able to account
for what variable calls happened and in what order.

In order to support that, this patch adds 8 callbacks:

	mock_set_variable_pre_hook()
	mock_set_variable_post_hook()
	mock_get_variable_pre_hook()
	mock_get_variable_post_hook()
	mock_get_next_variable_name_pre_hook()
	mock_get_next_variable_name_post_hook()
	mock_query_variable_info_pre_hook()
	mock_query_variable_info_post_hook()

The pre hooks each take the same arguments as their mocked namesake, and
they fire before any input validation.  The post hooks take an
additional EFI_STATUS argument.  The post hook fires immediately before
any return from the mocked namesake function.  For SetVariable(), the
arguments when the post hook fires are the current contents of the
variable if status is EFI_SUCCESS, and whatever arguments were passed in
if status is any other value.  For everything else, the arguments are
the correct results on EFI_SUCCESS, and whatever was passed in if status
is any other value.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: Add config table support
8c0a47e 
This adds a simple implementation of ST->ConfigurationTable,
ST->NumberOfTableEntries, and BS->InstallConfigurationTable  to our test
harness.

Currently it is limited at 1024 entries, but that should be well more
than enough for any tests we've currently considered.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
tests: Add a unit test for mok mirroring
c7eb2b7 
Test that our mok mirroring doesn't ever try to delete any variable that
it has previously created, and that it properly mirrors at least
MokList, MokListX, and SbatLevel, at least when variables actually work.

These tests will fail (rather a lot) without 7f64fd6.

Currently valgrind shows a memory leak in this code which is not
introduced in this patch series.  Since all of our memory is freed on
Exit() or when kernel does ExitBootServices(), this doesn't have any
significant repercussions.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela
mok: Fix memory leak in mok mirroring
5bd1269 
Currently valgrind shows a minor issue which is not introduced in this
patch series:

==2595397==
==2595397== HEAP SUMMARY:
==2595397==     in use at exit: 16,368 bytes in 48 blocks
==2595397==   total heap usage: 6,953 allocs, 6,905 frees, 9,146,749 bytes allocated
==2595397==
==2595397== 16,368 bytes in 48 blocks are definitely lost in loss record 1 of 1
==2595397==    at 0x4845464: calloc (vg_replace_malloc.c:1117)
==2595397==    by 0x4087F2: mock_efi_allocate_pool (test.c:72)
==2595397==    by 0x4098DE: UnknownInlinedFun (misc.c:33)
==2595397==    by 0x4098DE: AllocateZeroPool (misc.c:48)
==2595397==    by 0x403D40: get_variable_attr (variables.c:301)
==2595397==    by 0x4071C4: import_one_mok_state (mok.c:831)
==2595397==    by 0x4072F4: import_mok_state (mok.c:908)
==2595397==    by 0x407FA6: test_mok_mirror_0 (test-mok-mirror.c:205)
==2595397==    by 0x4035B2: main (test-mok-mirror.c:378)
==2595397==
==2595397== LEAK SUMMARY:
==2595397==    definitely lost: 16,368 bytes in 48 blocks
==2595397==    indirectly lost: 0 bytes in 0 blocks
==2595397==      possibly lost: 0 bytes in 0 blocks
==2595397==    still reachable: 0 bytes in 0 blocks
==2595397==         suppressed: 0 bytes in 0 blocks
==2595397==

This is because we're doing get_variable_attr() on the same variable
more than once and saving the value to our variables table.  Each
additional time we do so leaks the previous one.

This patch solves the issue by not getting the variable again if it's
already set in the table, and adds a test case to check if we're doing
get_variable() of any variety on the same variable more than once.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela removed the request for review from julian-klode September 7, 2021 20:53
@vathpela vathpela merged commit 35dc110 into rhboot:main Sep 7, 2021
@vathpela vathpela deleted the test-mok-mirror branch September 7, 2021 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frozencemetery frozencemetery frozencemetery left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vathpela @frozencemetery @lcp