| Age | Commit message (Collapse) | Author |
|---|
|
- one missing free
- one minor deadcode issue
- two unchecked allocations
- one debug hexdump of a variable we just freed
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
cov-analysis-linux64-2020.09 is a lot more successful than the older
versions at building, but it still has some... issues. Among them, it
is of the belief that this:
void
foo(char *fmt, ...)
{
__builtin_va_list ap;
__builtin_ms_va_start(ap, fmt); /* <- here */
...
}
is an uninitialized use of "ap".
This patch adds defined(__COVERITY__) to the list of criteria for using
sysv va lists, which it has no such confusion about.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Several places in e.g. MokManager and our console library use
ST->ConOut->ClearScreen directly, without checking for the existence of
a console output device.
This patch adds function to our console library to do that correctly,
instead of using the bug-prone ad hoc implementation everywhere.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Copying the value of datasize_in to two further variables and then using
all three randomly in the code makes it hard to read.
datasize_in is never changed in generate_hash() so we can do with this
parameter alone. Rename it to datasize.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
On Arch Linux, the GCC version of "limits.h" is in the "include-fixed" instead
of the "include" directory. It needs to be included in the include directories
list, otherwise attempting to compile the test suite fails with the following
error:
In file included from /usr/include/efivar/efivar-dp.h:22,
from /usr/include/efivar/efivar.h:238,
from include/test.h:51,
from shim.h:68,
from csv.c:6:
/usr/include/limits.h:124:16: fatal error: limits.h: No such file or directory
124 | # include_next <limits.h>
| ^~~~~~~~~~
compilation terminated.
|
|
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>
|
|
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 7f64fd6da9458b73c4.
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>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
This adds more mock functions that just return various EFI error codes
in the EFIAPI ABI.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
5f08e671e4eb 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>
|
|
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>
|
|
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>
|
|
There is no need to check the parameters of strntoken() twice.
Fixes: c7bb10cf154a ("Tidy up our string primitives...")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
This adds tests for all the cases we've documented in the
set_second_stage() comments. Each test checks that all of second_stage,
loader_str, and loader_str_size are set correctly.
Note that this adds a dependency on libefivar to build device paths to
test against.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This moves set_second_stage() and some of the helper functions it uses
out of shim.c, so that it's easier to write test cases for.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Some tests have some complex flows, and it's useful to be able to see
the call path when there's a failure.
This patch adds a very simple traceback printer, along with changing the
test build arguments to include more debug information.
The result you get from this traceback printer just gives you a function
name and the index into its .txt content, so to use it for more than
"which function calls which", you'll need to use eu-addr2line with the
output.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This test helper was conspicuously missing, so this patch just adds it
at the obvious place.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In some test cases, it may be useful to call libefi.a functions, such as
the device path parsing functions, which allocate pages via
BS->AllocatePool() or BS->AllocatePages.
This patch ads a simple mock implementation of those functions, as well
as the EFI_SYSTEM_TABLE, EFI_BOOT_SERVICES, and EFI_RUNTIME_SERVICES
variables *ST, *BS, and *RT (respectively), and initializes them before
the test cases run.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This allows us to use library functions from libefi.a in our test
programs.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
In some test cases, it's useful to be able to call some of the very
common stuff in gnu-efi's efilib.h (i.e. CompareGuid()), but including
that header itself is too big for me to tackle right now.
This patch adds a few more decls to test.h.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
pause() is a posix function, and having it named the same as this makes
it hard to include the asm.h header in some test cases.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The load options handling is quite complicated and tries to accomodate
several scenarios, but there are currently multiple issues:
- If the supplied LoadOptions is an EFI_LOAD_OPTION structure,
second_stage gets initialized to the entire contents of the OptionalData
field and load_options is initialized to NULL, which means it isn't
possible to pass additional options to the second stage loader (and it
looks like the intention is for this to be supported).
- If the supplied LoadOptions contains 2 or more strings, the code seems
to assume that shim was executed from the UEFI shell and that the first
argument is the path of the shim executable, so it's ignored. But this
breaks the ability to pass additional options to the second stage loader
from BDS on firmware implementations that initialize LoadOptions to just
the OptionalData field of the EFI_LOAD_OPTION, which is what EDK2 seems
to do.
This is moot anyway because this case (strings == 2) doesn't actually seem
to work, as nothing sets loader_len and therefore second_stage is not set
to the custom loader path.
- If the supplied LoadOptions contains a single string that isn't shim's
path, nothing sets loader_len and therefore second_stage isn't set at the
end of set_second_stage.
- set_second_stage replaces L' ' characters with L'\0' - whilst this is
useful to NULL terminate the path for the second stage, it doesn't seem
quite right to do this for the remaining LoadOptions data. Grub's
chainloader command supplies additional arguments as a NULL-terminated
space-delimited string via LoadOptions. Making it NULL-delimited seems to
be incompatible with the kernel's commandline handling, which wouldn't
work for scenarios where you might want to direct-boot a kernel image
(wrapped in systemd's EFI stub) from shim.
- handle_image passes the original LoadOptions to the second stage if
load_options is NULL, which means that the second stage currently always
gets shim's load options.
I've made an attempt to try to fix things. After the initial
checks in set_second_stage, it now does this:
- Tries to parse LoadOptions as an EFI_LOAD_OPTION in order to extract
the OptionalData if it is.
- If it's not an EFI_LOAD_OPTION, check if the first string is the
current shim path and ignore it if it is (the UEFI shell case).
- Split LoadOptions in to a single NULL terminated string (used to
initialize second_stage) and the unmodified remaining data (used to
initialize load_options and load_options_size).
I've also modified handle_image to always set LoadOptions and
LoadOptionsSize. If shim is executed with no options, or is only
executed with a single option to override the second stage loader
path, the second stage is executed with LoadOptions = NULL and
LoadOptionsSize = 0 now.
I've tested this on EDK2 and I can load a custom loader with extra
options from both BDS and the UEFI shell:
FS0:\> shimx64.efi test.efi
LoadOptionsSize: 0
LoadOptions: (null)
FS0:\> shimx64.efi test.efi
LoadOptionsSize: 0
LoadOptions: (null)
FS0:\> shimx64.efi test.efi foo bar
LoadOptionsSize: 16
LoadOptions: foo bar
|
|
Because a few shim builds were signed that did not properly initialize
the SBAT variable, and in doing so deleted valid SBAT variables, we need
to use a different name.
This changes the name from "SBAT" to "SbatLevel".
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
This moves the check for the SBAT variable's attributes and contents
into its own function, so that test cases can be written against it.
Signed-off-by: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
|
|
|
|
The MOK variable mirroring makes use of variable_create_esl, which
can only create a well-formed EFI_SIGNATURE_LIST containing a single
signature. Fix fill_esl and variable_create_esl to support creating
a EFI_SIGNATURE_LIST with one or more supplied EFI_SIGNATURE_DATA
structures.
Introduce variable_create_esl_with_one_signature and
fill_esl_with_one_signature for code that does want to create a
EFI_SIGNATURE_LIST containing a single signature constructed from
a supplied signature data buffer and owner GUID.
|
|
Don't check SHIM_UNIT_TEST.
This fixes conflicting declarations for __builtin_ms_va_list on amd64:
In file included from shim.h:47,
from test.c:10:
../include/system/stdarg.h:30:27: error: conflicting types for '__builtin_ms_va_list'
typedef __builtin_va_list __builtin_ms_va_list;
^~~~~~~~~~~~~~~~~~~~
cc1: note: previous declaration of '__builtin_ms_va_list' was here
In file included from shim.h:47,
from test-csv.c:9:
../include/system/stdarg.h:30:27: error: conflicting types for '__builtin_ms_va_list'
typedef __builtin_va_list __builtin_ms_va_list;
^~~~~~~~~~~~~~~~~~~~
cc1: note: previous declaration of '__builtin_ms_va_list' was here
In file included from shim.h:47,
from csv.c:6:
../include/system/stdarg.h:30:27: error: conflicting types for '__builtin_ms_va_list'
typedef __builtin_va_list __builtin_ms_va_list;
^~~~~~~~~~~~~~~~~~~~
cc1: note: previous declaration of '__builtin_ms_va_list' was here
Signed-off-by: Steve McIntyre <93sam@debian.org>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes it so that if you build with ENABLE_SHIM_DEVEL, the SBAT we
use is named SBAT_DEVEL instead of SBAT, and it's expected to have
EFI_VARIABLE_RUNTIME_ACCESS set.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
For some reason when we try to ever use the builtins, even with the
symbol there as a fallback, something goes horribly wrong somewhere
around here:
| (gdb) bt
| #0 strcmp (s1=0x7d492359 "MD5", s2=0x7d492359 "MD5") at include/system/string.h:57
| #1 0x000000007d460419 in getrn (lh=lh@entry=0x7e081318, data=data@entry=0x7e084398, rhash=rhash@entry=0x7f7c9268) at crypto/lhash/lhash.c:415
| #2 0x000000007d46076e in lh_insert (lh=0x7e081318, data=data@entry=0x7e084398) at crypto/lhash/lhash.c:188
| #3 0x000000007d43e027 in OBJ_NAME_add (name=name@entry=0x7d492359 "MD5", type=type@entry=1, data=data@entry=0x7d4ad3a0 <md5_md> "\004") at crypto/objects/o_names.c:202
As much as I love a Sisyphean challenge, in the interest of not having
bugs or time, this patch changes it to just not use them for anything
other than guaranteeing our implementations have the exact same types as
you would expect.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Be much more explicit about exactly which va_* stuff comes from which
ABI in both shim and gnu-efi. This fixes the problem where we see:
| (null):0:(null)() v->name:"(null)" v->rtname:"(null)"
| (null):0:(null)() v->data_size:0 v->data:0x0
and similar messages where everything is NULL.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
|
This adds test cases for the rest of our ASCII string functions. While
doing so, it fixes two minor bugs:
- strcasecmp() now handles utf8 correctly
- strncpy() no longer does the stpncpy() behavior of clearing leftover
buffer
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Note the one test case I'm not 100% sure about. Someone let me know.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This gets us the same working definition for VA_* va_* etc everywhere,
and it's the same definition edk2 is using.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This re-structures our includes so we can be sure everything is always
including all the system headers in a uniform, predictable way.
Temporarily it also adds a bunch of junk at all the places we use
variadic functions to specifically pick either the MS (cdecl) or ELF
ABIs.
I'm not 100% sure that's all correct (see later patch) but it's enough
to allow this to build.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This improves our static analysis targets by making them work better
with our make variables, and inhibits the use of ccache while building
those.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This get_variable_size() implementation success in either of two cases:
- EFI_SUCCESS with *lenp == 0 if the variable isn't found
- EFI_SUCCESS with *lenp > 0 on success
In the event of other errors, it returns them to you.
There's nothing particularly interesting about the set_variable()
or del_variable() implementation here.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
