| Age | Commit message (Collapse) | Author |
|---|
|
Some of our makefile bits are a mess, as you may have noticed, making
changes to them difficult to review.
This patch attempts to make some parts of them vaguely less of a mess,
in order to facilitate review of follow-up changes. To so it:
- coalesces feature flags, optimizations, -W{no-,}, -W{no-}error,
include directives, and define/undefine directives into (mostly)
separate groups.
- exports them as appropriate so the sub-makes can use them
- Makes sure we have -Wextra -Werror everywhere, but adds -Wno-foo and
-Wno-error=foo directives at the appropriate places to keep the net
warnings the same.
- makes the arch defines in Cryptlib and Cryptlib/OpenSSL use the
appropriate ones, with no attempt to make them less stupid, without
changing the overall order.
- coalesces the various includes, with no attempt to make them less
stupid, without changing the overall order.
- One giant glaring whitespace fix in Cryptlib/OpenSSL/Makefile
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We've been using "make -s", silent mode, which doesn't show us what
commands were invoked. Silent make means we can't see what goes wrong
when the wrong compiler or linker is getting invoked, or when it's being
invoked wrong.
This changes the CI to run it again without -s when it fails.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
When using "objcopy -O binary", it silently drops the sections without
"alloc" or "load" or the sections with "unload". Since we didn't set any
section flags for .sbat, it just contains the "readonly" flag and
objcopy ignored the section totally when generating EFI images for
ARM32 and AArch64.
This commit sets the common read-only data section flags to .sbat so
that objcopy would always copy the section to the final EFI image.
Signed-off-by: Gary Lin <glin@suse.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>
|
|
When grub2 invoked Exit() in AArch64 AAVMF, the VM crashed with the
following messsages:
Unloading driver at 0x000B7D7B000
Synchronous Exception at 0x00000000BF5D5E68
AllocatePool: failed to allocate 800 bytes
Synchronous Exception at 0x00000000BF5D5E68
The similar error also showed when I modified MokManager to call
gBS->Exit() at the end of efi_main(). However, if MokManager just
returned, the error never showed. One significant difference is
whether the loaded image was restored or not, and the firmware seems
to need the original ImageBase pointer to do clean-up.
To avoid the potential crash, this commit adds restore_loaded_image() so
that we can restore the loaded image both in start_image() and
do_exit().
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Fix a typo that inverted the check that validates that a CSV
column is not empty.
Signed-off-by: Thomas Frauendorfer | Miray Software <tf@miray.de>
|
|
The current code rejects the .sbat section if the VirtualSize and
SizeOfRawData fields of the section header aren't the same, but this
is too strict. The VirtualSize corresponds to the size of the SBAT
metadata and isn't necessarily a multiple of the file alignment.
The on-disk size (SizeOfRawData) is a multiple of the file alignment
and is zero padded. Make sure that the on-disk size is at least as
large as virtual size, and ignore the .sbat section if it isn't.
This should fix https://github.com/rhboot/shim/issues/281.
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
The raw text of the SBAT.md file is a bit of a mess, this patch
attempts to introduce some consistentcy to the text without affecting
the rendered Markdown.
The only content change was the addition of a missing period ('.') at
the end of sentence/paragraph; all of the other changes were purely
formatting changes.
Signed-off-by: Paul Moore <pmoore2@cisco.com>
|
|
Signed-off-by: Paul Moore <pmoore2@cisco.com>
|
|
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Two places we missed still have 0 for an sbat version - one doc and one
in our data csv.
This fixes those.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This commit fixes the following gcc warnings for casting.
sbat.c: In function 'verify_single_entry':
sbat.c:157:13: error: pointer targets in passing argument 1 of 'strcmp' differ in signedness [-Werror=pointer-sign]
if (strcmp(entry->component_name, sbat_var_entry->component_name) == 0) {
^~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
Cryptlib/Include/OpenSslSupport.h:299:16: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
int strcmp (const char *, const char *);
^~~~~~
sbat.c:157:36: error: pointer targets in passing argument 2 of 'strcmp' differ in signedness [-Werror=pointer-sign]
if (strcmp(entry->component_name, sbat_var_entry->component_name) == 0) {
^~~~~~~~~~~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
Cryptlib/Include/OpenSslSupport.h:299:16: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
int strcmp (const char *, const char *);
^~~~~~
sbat.c:165:19: error: pointer targets in passing argument 1 of 'AsciiStrDecimalToUintn' differ in signedness [-Werror=pointer-sign]
sbat_gen = atoi(entry->component_generation);
^
Cryptlib/Include/OpenSslSupport.h:380:66: note: in definition of macro 'atoi'
#define atoi(nptr) AsciiStrDecimalToUintn(nptr)
^~~~
In file included from Cryptlib/Include/OpenSslSupport.h:21:0,
from Cryptlib/Include/string.h:15,
from sbat.c:7:
Cryptlib/Library/BaseLib.h:9:7: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
UINTN AsciiStrDecimalToUintn(const char *String);
^~~~~~~~~~~~~~~~~~~~~~
In file included from Cryptlib/Include/string.h:15:0,
from sbat.c:7:
sbat.c:166:23: error: pointer targets in passing argument 1 of 'AsciiStrDecimalToUintn' differ in signedness [-Werror=pointer-sign]
sbat_var_gen = atoi(sbat_var_entry->component_generation);
^
Cryptlib/Include/OpenSslSupport.h:380:66: note: in definition of macro 'atoi'
#define atoi(nptr) AsciiStrDecimalToUintn(nptr)
^~~~
In file included from Cryptlib/Include/OpenSslSupport.h:21:0,
from Cryptlib/Include/string.h:15,
from sbat.c:7:
Cryptlib/Library/BaseLib.h:9:7: note: expected 'const char *' but argument is of type 'const CHAR8 * {aka const unsigned char *}'
UINTN AsciiStrDecimalToUintn(const char *String);
^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [<builtin>: sbat.o] Error 1
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This brings all the tests Chris Co wrote about parsing the .sbat section
back. Some of the actual test functions became redundant, and some new
ones were needed, but all of the actual test cases should be represented
here.
Note that building and running this test does not quite work yet /on
this branch/. In order to do that, we need some cleanups and
reorganizing that I don't want to push just yet, which can be found on
https://github.com/rhboot/shim/tree/test-reorg
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
[currently #if 0'd to oblivion by pjones]
Signed-off-by: Chris Co <chrco@microsoft.com>
|
|
This does a couple of straightforward tests on our CSV parser, and then
for good measure it does two with random data - one that's just random
data, one that's had all the zeros changed to nonzero values.
Note that building and running this test does not quite work yet /on
this branch/. In order to do that, we need some cleanups and
reorganizing that I don't want to push just yet, which can be found on
https://github.com/rhboot/shim/tree/test-reorg
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Note that building and running this test does not quite work yet /on
this branch/. In order to do that, we need some cleanups and
reorganizing that I don't want to push just yet, which can be found on
https://github.com/rhboot/shim/tree/test-reorg
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds a couple of make targets to do unit tests that are linked to
libc:
test-FOO : builds and runs test-FOO for any test-FOO.c
test : builds and runs all test-FOO tests
Note that building and running this test does not quite work yet /on
this branch/. In order to do that, we need some cleanups and
reorganizing that I don't want to push just yet, which can be found on
https://github.com/rhboot/shim/tree/test-reorg
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes our SBAT variable parser use the generic CSV parser, and also
changes its API slightly to produce a more testable intermediate
interface.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This makes the .sbat section parser use parse_csv_data(). It also
re-names a couple of the structs, because they were still too easy to
get lost in.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds a simple to use, one-function-call CSV parser that takes a
blob of data and gives you a linked list with an array of values.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This moves is_utf8_bom() to str.h, and also adds two #defines, UTF8_BOM
and UTF8_BOM_SIZE.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds list_size(), which tells us how many elements are in a list.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds constraints some of our compilers can check to all of our
string primitives, as well as adding implementations of:
- strdup - strdup
- stpcpy - stpcpy
- strnchrnul - strchrnul with a limit
- strntoken - a tokenizer
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This fixes the ifndef guard on NONNULL and __CONCAT3 and adds
definitions for:
- __CONCAT() for a##b with the intermediate tokenization step
- ALLOCFUNC for __malloc__
- DEPRECATED for __deprecated__
- PURE for __pure__
- RETURNS_NONNULL for __nonnull__
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
I'm adding even more of this stuff, and it's feeling pretty cluttered,
so this moves the scan-build and coverity makefiles into include/, where
we'll see them less.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This adds SBAT to our table of variables to mirror with our MoK state.
Currently it mirrors "SBAT" to a variable named "SbatRT", both using the
SHIM GUID.
Currently we enforce the current policy WRT these variables:
- we always delete SbatRT if it's present, for a couple of reasons:
- If we got here either something created it before us during boot,
which isn't a thing we believe anything should be doing, or it's an
NV variable, which it shouldn't be.
- we want to raise the error if it's NV+Authenticated
- we always delete SBAT (and do not mirror it) if it either
- doesn't have BS|NV set or
- does have RT set
- we're requiring !RT because we can't actually tell if it's an
authenticated variable or not, and we want to get the error if RT
is set and it is authenticated, because that means we've lost the
race between us and an attacker to create it.
- we always measure SBAT into PCR7 and add a log extension with the
measured hash
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Add a pile of documentation comments to help remember what all the
fields in our mok mirroring structure do.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This does two things:
- consolidates all our TPM event type #defines to one place
- uses EV_IPL instead of hard-coding 0xd
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
already enforces the alignment, clarify that objcopy only needs to
do the alignment in the SBAT spec.
|
|
Not everybody uses bash as /bin/sh, and the scripting here is safe
just using single brackets.
Signed-off-by: Steve McIntyre <93sam@debian.org>
|
|
|
|
At the default -Os optimization level, gcc emits ".text.startup"
and ".text.unlikely" sections for static initializers and noreturn
functions which end up in the intermediate ELF binary:
$ objdump -h build-x64/shimx64.efi.so
build-x64/shimx64.efi.so: file format elf64-x86-64
Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00046e7b 0000000000001000 0000000000001000 00001000 2**10
CONTENTS, ALLOC, LOAD, READONLY, CODE
1 .text.startup 00000118 0000000000047e7b 0000000000047e7b 00047e7b 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
2 .text.unlikely 00000046 0000000000047f93 0000000000047f93 00047f93 2**0
CONTENTS, ALLOC, LOAD, READONLY, CODE
3 .data 000315e8 0000000000048000 0000000000048000 00048000 2**9
These additional .text.* sections are omitted from the final PE/COFF
binary, resulting in a crash when processing the ctors. Taking a look at
_init_array in gdb:
(gdb) p/x &_init_array
$1 = 0x78510
(gdb) p/x &_init_array_end
$2 = 0x7851c
(gdb) x/x (void*)&_init_array
0x78510 <_init_array>: 0x00047e7b
(gdb) x/x (void*)(&_init_array)+8
0x78518 <_init_array+8>: 0x00000000
See that 0x00047e7b falls inside the padding between the .text and .data
sections:
$ objdump -h build-x64/shimx64.efi
build-x64/shimx64.efi: file format pei-x86-64
Sections:
Idx Name Size VMA LMA File off Algn
0 .text 00046e7b 0000000000001000 0000000000001000 00000400 2**10
CONTENTS, ALLOC, LOAD, READONLY, CODE
1 .data 000315e8 0000000000048000 0000000000048000 00047400 2**9
Adjust the linker script to merge the .text.startup and .text.unlikely
sections in to the .text section.
[edited by pjones to use .text.* instead of naming the sections
individually, and to sync up with what other arches have in .text]
|
|
Signed-off-by: Chris Coulson <chris.coulson@canonical.com>
|
|
"gcc -fanalyzer" found two NULL pointer checks we're missing in sbat.c:
include/str.h: In function ‘get_sbat_field.part.0’:
sbat.c:20:14: error: dereference of NULL ‘offset’ [CWE-476] [-Werror=analyzer-null-dereference]
20 | if (!*offset)
and
include/str.h: In function ‘parse_sbat’:
sbat.c:140:27: error: dereference of NULL ‘current’ [CWE-476] [-Werror=analyzer-null-dereference]
140 | } while (entry && *current != '\0');
Both are simple, and this patch fixes them.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This is needed for shim to verify itself when booting, to make sure that
shim binaries can't be executed anymore after been revoked by SBAT.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
A following patch will make shim to verify its .sbat section and it
should be done before doing the OpenSSL initialization. But having
the debugger attached may be useful at this point.
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
The parse_sbat() function is currently removing the last character of the
passed buffer, which will usually be a null-terminated string to parse.
There's no reason to do this and just take the whole size as specified by
the caller.
Reported-by: Chris Coulson <chris.coulson@canonical.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
|
|
handle_image() is quite huge and complex.
This patch moves the SBAT validation code from handle_image() to a new
function, handle_sbat().
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
On a typical boot we validate at least two binaries; parsing the SBAT
EFI variable each time, when it should not be changing, is not worth the
effort.
This patch moves the parsing out to some setup code, instead of doing it
during the verification stage.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Per Peter Jones suggestion, we will be flexible in what data we expect
while parsing the variable. Three fields are mandatory:
component_generation, component_name_size, component_name
However we also support adding comments and additional information to be
added after component name, with ',' as a separator. Those information
will be ignored and not used for verification purposes.
So:
grub,1
and
grub,1,wow,this,is,my,comment
will provide exactly same set of data for verification.
[0]: https://github.com/rhboot/shim/blob/main/SBAT.md
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
The struct sbat isn't doing anything and only has two fields so let's pass
pass those two to the functions directly instead of storing it in a struct.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Numbering the error messages in efi_main directly was a mistake, and the
following patches just make it more apparent.
This makes it an enum so we don't have to re-number at more than one
place when we add or remove them.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Currently, for an EV_EFI_VARIABLE_AUTHORITY event, the shim puts only
EFI_SIGNATURE_DATA.SignatureData in the VariableData field, but omits
EFI_SIGNATURE_DATA.SignatureOwner. According to reference implementation
in EDK2, the entire EFI_SIGNATURE_DATA is put into the VariableData
field, shown here:
https://github.com/tianocore/edk2/blob/master/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c#L1032
|
