| 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>
|
|
Requesting a keystroke when Secure Boot is not enabled and verbosity is
enabled is really annoying.
Signed-off-by: Renaud Métrich <rmetrich@redhat.com>
|
|
The buffer used to read the data in the CSV is declared as a stack variable
in the try_boot_csv() function, but a pointer to the arguments field of the
first boot option is stored in the global first_new_option_args variable.
Later, when is used set the arguments to boot the first entry, the variable
points to memory that no longer exists. This leads to booting an entry with
garbage as arguments instead of the correct value.
Reported-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
Introduce a new MOK variable called MokListTrustedRT. It allows an end-user
to decide if they want to trust MOKList keys within the soon to be booted
Linux kernel. This variable does not change any functionality within shim
itself. When Linux boots, if MokListTrustedRT is set and
EFI_VARIABLE_NON_VOLATILE is not set, keys in MokListRT are loaded into the
.machine keyring instead of the .platform keyring.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
In the bug2 section, the first Debian `.sbat` has a `grub,1`
component. But at this point in the story, `grub,1` has been revoked by
the update in the bug1 section. Updated it to `grub,2` so that it passes
that check.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We see various reports of boot failures because the generated
boot entries contain garbage/tagging that we do not expect, and
that we then parse as a second stage boot loader.
|
|
Simple refactoring that extracts the path checking on the given
loaded image. This will be useful to check if we were booted via
removable media path in other places.
|
|
The name of the SBAT UEFI variable changed from "SBAT" to "SbatLevel" in
27da4170f0fb30acde91a37e0256dfcfe76ea69e. Update the documentation to
match.
Signed-off-by: Nicholas Bishop <nicholasbishop@google.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>
|
|
There's been some discussion on how to handle machines without console
devices. The consensus so far has been that they should have dummy
ConOut implementations, but that means the first vendor to build a
machine without asking around is in for some surprises.
This patch makes the places where our console library uses ST->ConIn or
ST->ConOut check that they're present before doing so.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Seen on Dell PowerEdge R740 when booting with BOOTX64 constantly.
This patch keeps the behaviour previous to commit #1b30c2b by returning
the index of the "Linux" entry.
Then a check is made to find the entry in the current BootOrder:
- if it isn't there, prepend the entry and copy the rest (this enlarges
the BootOrder array by 1)
- if it's there, prepend the entry and copy all remaining entries
------------------------------------------------------------------------------
Example of outputs on a Dell PowerEdge R740:
- 0000 is BOOTX64 entry
- other entries are Dell's default ones (internal, no "Linux" there)
1. Entry not already existing in BootOrder
----
set_boot_order:486: Original nbootorder: 3
Original BootOrder: 0000 0003 0004
:
add_to_boot_list:578: device path: "HD(1,GPT,99D47E76-590F-48FD-8FD6-0A0CE790D635)/\EFI\redhat\shimx64.efi"
find_boot_option:454: Found boot entry "Boot0005" with label "Red Hat Enterprise Linux" for file "\EFI\redhat\shimx64.efi"
add_to_boot_list:623: New nbootorder: 4
BootOrder: 0005 0000 0003 0004
find_boot_options:937: Found directory named "Dell"
update_boot_order:509: nbootorder: 4
BootOrder: 0005 0000 0003 0004
----
2. Entry not existing at all
----
set_boot_order:486: Original nbootorder: 3
Original BootOrder: 0000 0001 0002
:
add_to_boot_list:578: device path: "HD(1,GPT,99D47E76-590F-48FD-8FD6-0A0CE790D635)/\EFI\redhat\shimx64.efi"
add_boot_option:245: Creating boot entry "Boot0005" with label "Red Hat Enterprise Linux" for file "\EFI\redhat\shimx64.efi"
add_boot_option:282: nbootorder: 4
BootOrder: 0005 0000 0001 0002
find_boot_options:937: Found directory named "Dell"
update_boot_order:509: nbootorder: 4
BootOrder: 0005 0000 0001 0002
----
3. Entry already existing in BootOrder
----
set_boot_order:486: Original nbootorder: 4
Original BootOrder: 0000 0005 0001 0002
:
add_to_boot_list:578: device path: "HD(1,GPT,99D47E76-590F-48FD-8FD6-0A0CE790D635)/\EFI\redhat\shimx64.efi"
find_boot_option:454: Found boot entry "Boot0005" with label "Red Hat Enterprise Linux" for file "\EFI\redhat\shimx64.efi"
add_to_boot_list:623: New nbootorder: 4
BootOrder: 0005 0000 0001 0002
find_boot_options:937: Found directory named "Dell"
update_boot_order:509: nbootorder: 4
BootOrder: 0005 0000 0001 0002
----
|
|
entry in optnum"
This reverts commit 1b30c2b9e5ee7d3e305a28a92805152d5cbfc9cb.
This commit was creating duplicated entries when the "Linux" entry was
not already in the BootOrder list, which may happen upon firmware reset.
|
|
Dump the load options before parsing them so that we can
see which things are failing to parse.
|
|
If the specified second stage loader does not exist (invalid
parameter), fall back to the DEFAULT_LOADER. This avoids failing
the boot on any garbage that made it through the load option parser
as a second stage loader name.
Bug-Ubuntu: https://bugs.launchpad.net/bugs/1937115
|
|
On Dell hardware booted in UEFI with option TPM 1.2 "On without Pre-Boot
Measurements", it appears that `tpm_log_event()` fails with Unsupported,
which causes Shim to abort due to believing it couldn't set up the
MokListRT, MokListXRT and SbatLevelRT variables.
This patch ignore the error when trying to write to the TPM and sets the
TPM as 'defective' to not try to write to it anymore.
Signed-off-by: Renaud Métrich <rmetrich@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.
|
|
The SBAT variable is defined as ASCII, but the SBAT section in a binary was defined as UTF-8. These should match.
Use ASCII rather than UTF-8, because naive parsing of UTF-8 could lead to unexpected results. For example the character 'ä' can be encoded as 0xe4 or as 0x61 0x0308, and these should be considered equivalent. The shim is not smart enough to do this. This could lead to missed verifications, if the variable and section use different encodings.
Define everything as ASCII. It's sad not to be able to have 🦀 in our bootloader names, and potentially annoying for vendor names as well, but oh well.
|
|
Data after a NUL byte should be ignored.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
.sbat sections contain user supplied data. We should not assume that it is
well formed. The last line feed might be missing or it might not be at the
end of the file. Instead one or more \0 might follow.
In parse_csv_data() variable 'line' is a pointer with a value between
the values of 'data' and 'data_end'.
There is no reason to check that it is non-zero after assigning it
from 'data' as we already check 'data'.
Instead at the beginning of the file and after each line we must check that
we have not reached the end of the file marked by a '\0' character.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
Signed-off-by: Esther Shimanovich eshimanovich@google.com
|
|
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>
|
|
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>
|
|
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>
|
|
Keep from cluttering up valgrind with allocations that aren't part of
the tested info (yet).
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
In 33db42def2ce, 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>
|
|
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>
|
|
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>
|
|
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: https://github.com/rhboot/shim/issues/386
Signed-off-by: Gary Lin <glin@suse.com>
|
|
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>
|
|
In the cloud, all boots are non-interactive with keyboard and console
access either typically not available or prohibited. Also clouds
always do firstboot via fallback. This currently results in an
unacceptable 5s boot delay whilst fallback offers interactive reset
options that cannot be actioned.
In Ubuntu, we'd like to make fallback noninteractive by default
without any boot delays, due to bootspeed impact on firstboot of the
preinstalled images.
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/shim/+bug/1922581
Signed-off-by: Dimitri John Ledkov <xnox@ubuntu.com>
|
|
Now that we are iterating over all EFI variables to look for duplicate
boot entries, we should use a dynamic buffer, so if the firmware tells
us the buffer is too small for the next variable name we can re-allocate
it to a big enough size.
Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
|
|
When listing all variables checking for duplicates, print which error
was returned if not EFI_NOT_FOUND (which simply means reached the end of
the list).
Signed-off-by: João Paulo Rechi Vita <jprvita@endlessos.org>
|
|
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>
|
