The following commit:
commit 4aac8a1179
Author: Gary Ching-Pang Lin <glin@suse.com>
Date: Thu Mar 6 10:57:02 2014 +0800
[fallback] Fix the data size for boot option comparison
corrected the data size used for comparison, but also reduced the
allocation so it doesn't include the trailing UTF16LE '\0\0' at the
end of the string, with the result that the trailer of the buffer
containing the string is overwritten, which OVMF detects as memory
corruption.
Increase the size of the storage buffer in a few places to correct
this problem.
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gary Ching-Pang Lin <glin@suse.com>
fallback.c: In function ‘update_boot_order’:
fallback.c:334:17: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (j = 0 ; j < size / sizeof (CHAR16); j++)
^
fallback.c: In function ‘add_to_boot_list’:
fallback.c:402:16: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
for (i = 0; i < s; i++) {
^
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
System services haven't been hooked if we're not in secure mode, so
do_exit() will never be called. In this case shim never gets control
once grub exits, which means if booting fails and the firmware tries
another boot option, it'll attempt to talk to the shim protocol we
installed.
This is wrong, because it is allowed to have been cleared from ram at
this time, since the task it's under has exited.
So just don't install the protocols when we're not enforcing.
This version also has a message and a 2-second stall after calling
start_image(), so that we can tell if we are on the expected return path
of our execution flow.
Turns out a) the codegen on aarch64 generates code that has real
alignment needs, and b) if we check the length of discardable sections
before discarding them, we error for no reason.
So do the error checking in the right order, and always enforce some
alignment because we know we have to.
Signed-off-by: Peter Jones <pjones@redhat.com>
This check is for end == NULL but was meant to be *end == '\0'. Without
this change, we'll pass a plausibly bad address (i.e. one with no ']' at
the end) to Mtftp(... READ_FILE ...), which should fail correctly, but
our error messaging will be inconsistent.
Signed-off-by: Peter Jones <pjones@redhat.com>
I mistakenly added CryptPkcs7VerifyNull.c which may make Pkcs7Verify
always return FALSE. Besides CryptPkcs7VerifyNull.c, there are some
functions we would never use. This commit removes those files to
avoid any potential trouble.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
We replaced the build key with an empty file while compiling shim
for our distro. Skip the verification with the empty build key
since this makes no sense.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
Find the relocations based on the *file* address in the old binary,
because it's only the same as the virtual address some of the time.
Also perform some extra validation before processing it, and don't bail
out in /error/ if both ReloceBase and RelocEnd are null - that condition
is fine.
Signed-off-by: Peter Jones <pjones@redhat.com>
Revert "Do the same for ia32..."
and "Generate a sane PE header on shim, fallback, and MokManager."
This reverts commit 6744a7ef8e.
and commit 0e7ba5947e.
These are premature and I can do this without such drastic measures.
Signed-off-by: Peter Jones <pjones@redhat.com>
Once again, on ia32 this time, we see:
00000120 47 84 00 00 0a 00 00 00 00 00 00 00 00 00 00 00 |G...............|
Which is where the pointer on ia32 for the Base Relocation Table should
be. It points to 0x8447, which isn't a particularly reasonable address as
numbers go, and happens to have this data there:
00008440 6f 00 6e 00 66 00 69 00 67 00 75 00 72 00 65 00 |o.n.f.i.g.u.r.e.|
00008450 00 00 49 00 50 00 76 00 36 00 28 00 00 00 2c 00 |..I.P.v.6.(...,.|
00008460 25 00 73 00 2c 00 00 00 29 00 00 00 25 00 64 00 |%.s.,...)...%.d.|
00008470 2e 00 25 00 64 00 2e 00 25 00 64 00 2e 00 25 00 |..%.d...%.d...%.|
00008480 64 00 00 00 44 00 48 00 43 00 50 00 00 00 49 00 |d...D.H.C.P...I.|
00008490 50 00 76 00 34 00 28 00 00 00 2c 00 25 00 73 00 |P.v.4.(...,.%.s.|
And so that table is, in theory, this part:
00008447 00 67 00 75 00 72 00 65 00 | .g.u.r.e.|
00008450 00 |. |
Which is pretty clearly not a pointer table of any kind.
So give ia32 the same treatment as x86_64, and now all arches work basically
the same.
Signed-off-by: Peter Jones <pjones@redhat.com>
It turns out a7249a65 was masking a second problem - on some binaries,
when we actually don't have any base relocations at all, binutils'
"objcopy --target efi-app-x86_64" is generating a PE header with a base
relocations pointer that happily points into the middle of our text
section. So with shim processing base relocations correctly, it refuses
to load those binaries.
For example, on one binary I just built:
00000130 00 a0 00 00 0a 00 00 00 00 00 00 00 00 00 00 00 |................|
which says there's a Base Relocation Table at 0xa000 that's 0xa bytes long.
That's here:
0000a000 58 00 29 00 00 00 00 00 48 00 44 00 28 00 50 00 |X.).....H.D.(.P.|
0000a010 61 00 72 00 74 00 25 00 64 00 2c 00 53 00 69 00 |a.r.t.%.d.,.S.i.|
0000a020 67 00 25 00 67 00 29 00 00 00 00 00 00 00 00 00 |g.%.g.).........|
0000a030 48 00 44 00 28 00 50 00 61 00 72 00 74 00 25 00 |H.D.(.P.a.r.t.%.|
So the table is:
0000a000 58 00 29 00 00 00 00 00 48 00 |X.).....H. |
That wouldn't be so bad, except those binaries are MokManager.efi,
fallback.efi, and shim.efi, and sometimes they're .reloc, which we're
actually trying to handle correctly now because grub builds with a real
and valid .reloc table. So though I didn't think there was any hair
left on this yak, more shaving ensues.
With this change, instead of letting objcopy do whatever it likes, we
switch to "-O binary" and merely link in a header that's appropriate for
our binaries. This is the same method Ard wrote for aarch64, and it
seems to work fine in either place (modulo some minor changes.)
At some point this should be merged into gnu-efi instead of carrying our
own crt0-efi-x86_64.S, but that's a less immediate problem.
I did not need this problem.
Signed-off-by: Peter Jones <pjones@redhat.com>
When I merged 4bfb13d and fixed the conflicts, I managed to make the
in_protocol test exactly backwards, so that's why we don't currently see
error messages.
Signed-off-by: Peter Jones <pjones@redhat.com>
Actually check the size of our vendor cert quite early, so that there's
no confusion as to what's going on.
This isn't strictly necessary, in that in all cases if vendor_cert_size
is 0, then AuthenticodeVerify -> Pkcs7Verify() -> d2i_X509() will result
in a NULL "Cert", and it will return FALSE, and we'll reject the
signature, but better to avoid all that code in the first place. Belt
and suspenders and whatnot.
Based on a patch from https://github.com/TBOpen .
Signed-off-by: Peter Jones <pjones@redhat.com>
I screwed one of these up when working on 750584c, and it's a real pain
to figure out, so that means we should be validating them.
Signed-off-by: Peter Jones <pjones@redhat.com>
This is mostly based on a patch (https://github.com/mjg59/shim/issues/30)
from https://github.com/TBOpen , which refactors our __LP64__
tests to be tests of the header magic instead. I've simplified things
by using what we've pre-loaded into "context" and making some helper
functions so the conditionals in most of the code say what they do,
instead of how they work.
Note that we're only allowing that from in_protocol's loader - that is,
we'll let 64-bit grub load a 32-bit kernel or 32-bit grub load a 64-bit
kernel, but 32-bit shim isn't loading a 64-bit grub.
Signed-off-by: Peter Jones <pjones@redhat.com>
Currently when we process base relocations, we get the correct Data
Directory pointer from the headers (context->RelocDir), and that header
has been copied into our pristine allocated image when we copied up to
SizeOfHeaders. But the data it points to has not been mirrored in to
the new image, so it is whatever data AllocPool() gave us.
This patch changes relocate_coff() to refer to the base relocation table
from the image we loaded from disk, but apply the fixups to the new
copy.
I have no idea how x86_64 worked without this, but I can't make aarch64
work without it. I also don't know how Ard or Leif have seen aarch64
work. Maybe they haven't? Leif indicated on irc that they may have
only tested shim with simple "hello world" applications from gnu-efi;
they are certainly much less complex than grub.efi, and are generated
through a different linking process.
My only theory is that we're getting recycled data there pretty reliably
that just makes us /not/ process any relocations, but since our
ImageBase is 0, and I don't think we ever load grub with 0 as its base
virtual address, that doesn't follow. I'm open to any other ideas
anybody has.
I do know that on x86_64 (and presumably aarch64 as well), we don't
actually start seeing *symptoms* of this bug until the first chunk[0] of
94c9a77f is applied[1]. Once that is applied, relocate_coff() starts
seeing zero[2] for both RelocBase->VirtualAddress and
RelocBase->SizeOfBlock, because RelocBase is a (generated, relative)
pointer that only makes sense in the context of the original binary, not
our partial copy. Since RelocBase->SizeOfBlock is tested first,
relocate_base() gives us "Reloc block size is invalid"[3] and returns
EFI_UNSUPPORTED. At that point shim exits with an error.
[0] The second chunk of 94c9a77f patch makes no difference on this
issue.
[1] I don't see why at all.
[2] Which could really be any value since it's AllocatePool() and not
AllocateZeroPool() results, but 0 is all I've observed; I think
AllocatePool() has simply never recycled any memory in my test
cases.
[3] which is silent because perror() tries to avoid talking because that
has caused much crashing in the past; work needs to go in to 0.9 for
this.
Signed-off-by: Peter Jones <pjones@redhat.com>
Since in theory you could, for example, get an x86_64 binary signed that
also behaves as an ARM executable, we should be checking this before
people build on other architectures.
Signed-off-by: Peter Jones <pjones@redhat.com>
On aarch64 due to some terrifying include chain we wind up with
Cryptlib's definition of exit here. I'm not a glutton for punishment,
so I'm just changing the name so it's not coliding.
Signed-off-by: Peter Jones <pjones@redhat.com>
On archs where no EFI aware objcopy is available, the generated PE/COFF
header contains a .reloc section which is completely empty. Handle this by
- returning early from relocate_coff() with EFI_SUCCESS,
- ignoring discardable sections in the section loader.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
We don't need to .data entries; the second one should be .data*. He's
since fixed this in his tree, but I'd already pulled it and pushed to
master.
Signed-off-by: Peter Jones <pjones@redhat.com>
This patch cleans up and refactors the Makefiles to better allow new
architectures to be added:
- remove unused Makefile definitions
- import Makefile definitions from top level rather than redefining
- move x86 specific CFLAGS to inside ifeq() blocks
- remove x86 inline asm
- allow $(FORMAT) to be overridden: this is necessary as there exists no
EFI or PE/COFF aware objcopy for ARM
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Prevent unhook_system_services() from dereferencing a NULL systab, which
may occur if hook_system_services() has never been called.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Upstream GNU-EFI contains changes to efistdarg.h resulting in the va_start,
va_arg and va_end macros to be #defined unconditionally. Make sure we #undef
them before overriding the definitions.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
MokSBState and MokDBState are just 1 byte variables, so a UINT8
local variable is sufficient to include the content.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
Conflicts:
shim.c
There are functions defined in lib to check the secure variables.
Use the functions to shun the duplicate code.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
Conflicts:
shim.c
When grub2 invokes the functions of shim protocol in gfx mode,
OutputString in shim could distort the screen.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
Conflicts:
shim.c
(modified by pjones to include some newer Prints that weren't there when
Gary did the initial work here.)
If ca.crt was added into the certificate database, ca.crt would be the first
certificate in the signature. Because shim couldn't verify ca.crt with the
embedded shim.cer, it failed to load MokManager.efi.signed and
fallback.efi.signed.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
