Verified Commit 25edab96 authored by Sherif Nagy's avatar Sherif Nagy
Browse files

cherry-pick git format-patch 15.4..4d64389c6c941d21548b06423b8131c872e3c3c7...

cherry-pick git format-patch 15.4..4d64389c6c941d21548b06423b8131c872e3c3c7 and bump version to .1.2
parent 9d82d43b
......@@ -46,6 +46,18 @@ add {
file: "ROCKY/_supporting/0007-Relax-the-check-for-import_mok_state.patch"
}
add {
file: "ROCKY/_supporting/0008-SBAT.md-trivial-fixes.patch"
}
add {
file: "ROCKY/_supporting/0009-SBAT.md-fix-will-should.patch"
}
add {
file: "ROCKY/_supporting/0010-shim-another-attempt-to-fix-load-options-handling.patch"
}
patch {
file: "ROCKY/_supporting/0001-Adding-rocky-linux-CA.patch"
......@@ -61,7 +73,7 @@ spec_change {
append {
field: "Release"
value: ".1.1"
value: ".1.2"
}
file {
......@@ -112,6 +124,24 @@ spec_change {
add: true
}
file {
name: "0008-SBAT.md-trivial-fixes.patch"
type: Patch
add: true
}
file {
name: "0009-SBAT.md-fix-will-should.patch"
type: Patch
add: true
}
file {
name: "0010-shim-another-attempt-to-fix-load-options-handling.patch"
type: Patch
add: true
}
file {
name: "redhatsecurebootca5.cer"
type: Source
......@@ -124,6 +154,12 @@ spec_change {
delete: true
}
changelog {
author_name: "Sherif Nagy"
author_email: "sherif@rockylinux.org"
message: "cherry-pick patches for shim-reivew git 15.4..4d64389c6c941d21548b06423b8131c872e3c3c7 and bump version to .1.2"
}
changelog {
author_name: "Sherif Nagy"
author_email: "sherif@rockylinux.org"
......
From 822d07ad4f07ef66fe447a130e1027c88d02a394 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Thu, 8 Apr 2021 22:39:02 -0700
Subject: [PATCH 1/7] Fix handling of ignore_db and user_insecure_mode
Subject: [PATCH 01/10] Fix handling of ignore_db and user_insecure_mode
In 65be350308783a8ef537246c8ad0545b4e6ad069, import_mok_state() is split
up into a function that manages the whole mok state, and one that
......
From a0f701501f73a0aabd1ef8d568183d05611b0a52 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 31 Mar 2021 09:44:53 -0400
Subject: [PATCH 2/7] shim-15.4 branch: update .gitmodules to point at
Subject: [PATCH 02/10] shim-15.4 branch: update .gitmodules to point at
shim-15.4 in gnu-efi
This is purely superficial, as the commit points at the shim-15.4 branch
......
From 5b3ca0d2f7b5f425ba1a14db8ce98b8d95a2f89f Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 31 Mar 2021 14:54:52 -0400
Subject: [PATCH 3/7] Fix a broken file header on ia32
Subject: [PATCH 03/10] Fix a broken file header on ia32
Commit c6281c6a195edee61185 needs to have included a ". = ALIGN(4096)"
directive before .reloc, but fails to do so.
......
From 4068fd42c891ea6ebdec056f461babc6e4048844 Mon Sep 17 00:00:00 2001
From: Gary Lin <glin@suse.com>
Date: Thu, 8 Apr 2021 16:23:03 +0800
Subject: [PATCH 4/7] mok: allocate MOK config table as BootServicesData
Subject: [PATCH 04/10] mok: allocate MOK config table as BootServicesData
Linux kernel is picky when reserving the memory for x86 and it only
expects BootServicesData:
......
From 493bd940e5c6e28e673034687de7adef9529efff Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Sat, 10 Apr 2021 16:05:23 -0400
Subject: [PATCH 5/7] Don't call QueryVariableInfo() on EFI 1.10 machines
Subject: [PATCH 05/10] Don't call QueryVariableInfo() on EFI 1.10 machines
The EFI 1.10 spec (and presumably earlier revisions as well) didn't have
RT->QueryVariableInfo(), and on Chris Murphy's MacBookPro8,2 , that
......
From 05875f3aed1c90fe071c66de05744ca2bcbc2b9e Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 13 May 2021 20:42:18 -0400
Subject: [PATCH 6/7] Post-process our PE to be sure.
Subject: [PATCH 06/10] Post-process our PE to be sure.
On some versions of binutils[0], including binutils-2.23.52.0.1-55.el7,
do not correctly initialize the data when computing the PE optional
......
From 9f973e4e95b1136b8c98051dbbdb1773072cc998 Mon Sep 17 00:00:00 2001
From: Gary Lin <glin@suse.com>
Date: Tue, 11 May 2021 10:41:43 +0800
Subject: [PATCH 7/7] Relax the check for import_mok_state()
Subject: [PATCH 07/10] Relax the check for import_mok_state()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
......
From 9a8a6cdb5e862648ee663eee6124bed05208639a Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge@hallyn.com>
Date: Wed, 14 Jul 2021 07:49:34 -0500
Subject: [PATCH 08/10] SBAT.md: trivial fixes
1. Use : instead of , to separate a list.
2. Fix spelling of therefore.
3. Pull unrelated clause out of parenthesized clause.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
---
SBAT.md | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/SBAT.md b/SBAT.md
index 1a5ecad..c1edf15 100644
--- a/SBAT.md
+++ b/SBAT.md
@@ -6,7 +6,7 @@ In the PC ecosystem, [UEFI Secure Boot](https://docs.microsoft.com/en-us/windows
is typically configured to trust 2 authorities for signing UEFI boot code, the
Microsoft UEFI Certificate Authority (CA) and Windows CA. When malicious or
security compromised code is detected, 2 revocation mechanisms are provided by
-compatible UEFI implementations, signing certificate or image hash. The UEFI
+compatible UEFI implementations: signing certificate or image hash. The UEFI
Specification does not provides any well tested additional revocation
mechanisms.
@@ -269,7 +269,7 @@ that the global generation numbers will eventually move forward and exclude
those products from booting on a UEFI Secure Boot enabled system. However a
product made up of GRUB and a closed source kernel is just as conceivable. In
that case the kernel version may never move forward once the product reaches
-its end of support. Therefor it is recommended that the product-specific
+its end of support. Therefore it is recommended that the product-specific
generation number be incremented past the latest one shown in any binary for
that product, effectively disabling that product on UEFI Secure Boot enabled
systems.
@@ -309,7 +309,7 @@ compromise.
The initial SBAT implementation will add SBAT metadata to Shim and GRUB and
enforce SBAT on all components labeled with it. Until a component (e.g. the
-Linux kernel gains SBAT metadata) it can not be revoked via SBAT, but only by
+Linux kernel) gains SBAT metadata it can not be revoked via SBAT, but only by
revoking the keys signing that component. These keys will should live in
separate, product-specific signed PE files that contain **only** the
certificate and SBAT metadata for the key files. These key files can then be
--
2.32.0
From 0f40cb0d08798ed7557887958b382a42253c715d Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge@hallyn.com>
Date: Wed, 14 Jul 2021 08:01:48 -0500
Subject: [PATCH 09/10] SBAT.md: fix "will should"
Use the stronger "will" rather than "will should". I'm not sure based on
what's there, but suspect "must" would be appropriate instead?
Signed-off-by: Serge Hallyn <serge@hallyn.com>
---
SBAT.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/SBAT.md b/SBAT.md
index c1edf15..5152a90 100644
--- a/SBAT.md
+++ b/SBAT.md
@@ -310,7 +310,7 @@ compromise.
The initial SBAT implementation will add SBAT metadata to Shim and GRUB and
enforce SBAT on all components labeled with it. Until a component (e.g. the
Linux kernel) gains SBAT metadata it can not be revoked via SBAT, but only by
-revoking the keys signing that component. These keys will should live in
+revoking the keys signing that component. These keys will live in
separate, product-specific signed PE files that contain **only** the
certificate and SBAT metadata for the key files. These key files can then be
revoked via SBAT in order to invalidate and replace a specific key. While
--
2.32.0
From 4d64389c6c941d21548b06423b8131c872e3c3c7 Mon Sep 17 00:00:00 2001
From: Chris Coulson <chris.coulson@canonical.com>
Date: Mon, 7 Jun 2021 16:34:18 +0100
Subject: [PATCH 10/10] shim: another attempt to fix load options handling
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
---
include/ucs2.h | 27 -------
pe.c | 6 +-
shim.c | 200 ++++++++++++++++++++++---------------------------
3 files changed, 92 insertions(+), 141 deletions(-)
diff --git a/include/ucs2.h b/include/ucs2.h
index e43c341..ee038ce 100644
--- a/include/ucs2.h
+++ b/include/ucs2.h
@@ -81,31 +81,4 @@ is_all_nuls(UINT8 *data, UINTN data_size)
return true;
}
-static inline UINTN
-__attribute__((__unused__))
-count_ucs2_strings(UINT8 *data, UINTN data_size)
-{
- UINTN pos = 0;
- UINTN last_nul_pos = 0;
- UINTN num_nuls = 0;
- UINTN i;
-
- if (data_size % 2 != 0)
- return 0;
-
- for (i = pos; i < data_size; i++) {
- if (i % 2 != 0) {
- if (data[i] != 0)
- return 0;
- } else if (data[i] == 0) {
- last_nul_pos = i;
- num_nuls++;
- }
- pos = i;
- }
- if (num_nuls > 0 && last_nul_pos != pos - 1)
- return 0;
- return num_nuls;
-}
-
#endif /* SHIM_UCS2_H */
diff --git a/pe.c b/pe.c
index 365e32a..13bc397 100644
--- a/pe.c
+++ b/pe.c
@@ -1144,10 +1144,8 @@ handle_image (void *data, unsigned int datasize,
li->ImageSize = context.ImageSize;
/* Pass the load options to the second stage loader */
- if ( load_options ) {
- li->LoadOptions = load_options;
- li->LoadOptionsSize = load_options_size;
- }
+ li->LoadOptions = load_options;
+ li->LoadOptionsSize = load_options_size;
if (!found_entry_point) {
perror(L"Entry point is not within sections\n");
diff --git a/shim.c b/shim.c
index 40e4894..ecf6ee5 100644
--- a/shim.c
+++ b/shim.c
@@ -1241,9 +1241,13 @@ EFI_STATUS init_grub(EFI_HANDLE image_handle)
return efi_status;
}
+/*
+ * Extract the OptionalData and OptionalData fields from an
+ * EFI_LOAD_OPTION.
+ */
static inline EFI_STATUS
-get_load_option_optional_data(UINT8 *data, UINTN data_size,
- UINT8 **od, UINTN *ods)
+get_load_option_optional_data(VOID *data, UINT32 data_size,
+ VOID **od, UINT32 *ods)
{
/*
* If it's not at least Attributes + FilePathListLength +
@@ -1253,7 +1257,8 @@ get_load_option_optional_data(UINT8 *data, UINTN data_size,
if (data_size < (sizeof(UINT32) + sizeof(UINT16) + 2 + 4))
return EFI_INVALID_PARAMETER;
- UINT8 *cur = data + sizeof(UINT32);
+ UINT8 *start = (UINT8 *)data;
+ UINT8 *cur = start + sizeof(UINT32);
UINT16 fplistlen = *(UINT16 *)cur;
/*
* If there's not enough space for the file path list and the
@@ -1263,8 +1268,8 @@ get_load_option_optional_data(UINT8 *data, UINTN data_size,
return EFI_INVALID_PARAMETER;
cur += sizeof(UINT16);
- UINTN limit = data_size - (cur - data) - fplistlen;
- UINTN i;
+ UINT32 limit = data_size - (cur - start) - fplistlen;
+ UINT32 i;
for (i = 0; i < limit ; i++) {
/* If the description isn't valid UCS2-LE, it's not valid. */
if (i % 2 != 0) {
@@ -1380,6 +1385,57 @@ done:
return ret;
}
+/*
+ * Split the supplied load options in to a NULL terminated
+ * string representing the path of the second stage loader,
+ * and return a pointer to the remaining load options data
+ * and its remaining size.
+ *
+ * This expects the supplied load options to begin with a
+ * string that is either NULL terminated or terminated with
+ * a space and some optional data. It will return NULL if
+ * the supplied load options contains no spaces or NULL
+ * terminators.
+ */
+static CHAR16 *
+split_load_options(VOID *in, UINT32 in_size,
+ VOID **remaining,
+ UINT32 *remaining_size) {
+ UINTN i;
+ CHAR16 *arg0 = NULL;
+ CHAR16 *start = (CHAR16 *)in;
+
+ /* Skip spaces */
+ for (i = 0; i < in_size / sizeof(CHAR16); i++) {
+ if (*start != L' ')
+ break;
+
+ start++;
+ }
+
+ in_size -= ((VOID *)start - in);
+
+ /*
+ * Ensure that the first argument is NULL terminated by
+ * replacing L' ' with L'\0'.
+ */
+ for (i = 0; i < in_size / sizeof(CHAR16); i++) {
+ if (start[i] == L' ' || start[i] == L'\0') {
+ start[i] = L'\0';
+ arg0 = (CHAR16 *)start;
+ break;
+ }
+ }
+
+ if (arg0) {
+ UINTN skip = i + 1;
+ *remaining_size = in_size - (skip * sizeof(CHAR16));
+ *remaining = *remaining_size > 0 ? start + skip : NULL;
+ }
+
+ return arg0;
+}
+
/*
* Check the load options to specify the second stage loader
*/
@@ -1387,20 +1443,11 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle)
{
EFI_STATUS efi_status;
EFI_LOADED_IMAGE *li = NULL;
- CHAR16 *start = NULL;
- UINTN remaining_size = 0;
+ VOID *remaining = NULL;
+ UINT32 remaining_size;
CHAR16 *loader_str = NULL;
- UINTN loader_len = 0;
- unsigned int i;
- UINTN second_stage_len;
- second_stage_len = (StrLen(DEFAULT_LOADER) + 1) * sizeof(CHAR16);
- second_stage = AllocatePool(second_stage_len);
- if (!second_stage) {
- perror(L"Could not allocate %lu bytes\n", second_stage_len);
- return EFI_OUT_OF_RESOURCES;
- }
- StrCpy(second_stage, DEFAULT_LOADER);
+ second_stage = DEFAULT_LOADER;
load_options = NULL;
load_options_size = 0;
@@ -1499,105 +1546,44 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle)
return EFI_SUCCESS;
/*
- * Check and see if this is just a list of strings. If it's an
- * EFI_LOAD_OPTION, it'll be 0, since we know EndEntire device path
- * won't pass muster as UCS2-LE.
- *
- * If there are 3 strings, we're launched from the shell most likely,
- * But we actually only care about the second one.
+ * See if this is an EFI_LOAD_OPTION and extract the optional
+ * data if it is. This will return an error if it is not a valid
+ * EFI_LOAD_OPTION.
*/
- UINTN strings = count_ucs2_strings(li->LoadOptions,
- li->LoadOptionsSize);
-
- /*
- * In some cases we get strings == 1 because BDS is using L' ' as the
- * delimeter:
- * 0000:74 00 65 00 73 00 74 00 2E 00 65 00 66 00 69 00 t.e.s.t...e.f.i.
- * 0016:20 00 6F 00 6E 00 65 00 20 00 74 00 77 00 6F 00 ..o.n.e...t.w.o.
- * 0032:20 00 74 00 68 00 72 00 65 00 65 00 00 00 ..t.h.r.e.e...
- *
- * If so replace it with NULs since the code already handles that
- * case.
- */
- if (strings == 1) {
- UINT16 *cur = start = li->LoadOptions;
-
- /* replace L' ' with L'\0' if we find any */
- for (i = 0; i < li->LoadOptionsSize / 2; i++) {
- if (cur[i] == L' ')
- cur[i] = L'\0';
- }
-
- /* redo the string count */
- strings = count_ucs2_strings(li->LoadOptions,
- li->LoadOptionsSize);
- }
-
- /*
- * If it's not string data, try it as an EFI_LOAD_OPTION.
- */
- if (strings == 0) {
- /*
- * We at least didn't find /enough/ strings. See if it works
- * as an EFI_LOAD_OPTION.
- */
- efi_status = get_load_option_optional_data(li->LoadOptions,
- li->LoadOptionsSize,
- (UINT8 **)&start,
- &loader_len);
- if (EFI_ERROR(efi_status))
- return EFI_SUCCESS;
-
- remaining_size = 0;
- } else if (strings >= 2) {
+ efi_status = get_load_option_optional_data(li->LoadOptions,
+ li->LoadOptionsSize,
+ &li->LoadOptions,
+ &li->LoadOptionsSize);
+ if (EFI_ERROR(efi_status)) {
/*
+ * it's not an EFI_LOAD_OPTION, so it's probably just a string
+ * or list of strings.
+ *
* UEFI shell copies the whole line of the command into
- * LoadOptions. We ignore the string before the first L'\0',
- * i.e. the name of this program.
+ * LoadOptions. We ignore the first string, i.e. the name of this
+ * program in this case.
*/
- UINT16 *cur = li->LoadOptions;
- for (i = 1; i < li->LoadOptionsSize / 2; i++) {
- if (cur[i - 1] == L'\0') {
- start = &cur[i];
- remaining_size = li->LoadOptionsSize - (i * 2);
- break;
- }
+ CHAR16 *loader_str = split_load_options(li->LoadOptions,
+ li->LoadOptionsSize,
+ &remaining,
+ &remaining_size);
+
+ if (loader_str && is_our_path(li, loader_str)) {
+ li->LoadOptions = remaining;
+ li->LoadOptionsSize = remaining_size;
}
-
- remaining_size -= i * 2 + 2;
- } else if (strings == 1 && is_our_path(li, start)) {
- /*
- * And then I found a version of BDS that gives us our own path
- * in LoadOptions:
-
-77162C58 5c 00 45 00 46 00 49 00 |\.E.F.I.|
-77162C60 5c 00 42 00 4f 00 4f 00 54 00 5c 00 42 00 4f 00 |\.B.O.O.T.\.B.O.|
-77162C70 4f 00 54 00 58 00 36 00 34 00 2e 00 45 00 46 00 |O.T.X.6.4...E.F.|
-77162C80 49 00 00 00 |I...|
-
- * which is just cruel... So yeah, just don't use it.
- */
- return EFI_SUCCESS;
}
+ loader_str = split_load_options(li->LoadOptions, li->LoadOptionsSize,
+ &remaining, &remaining_size);
+
/*
* Set up the name of the alternative loader and the LoadOptions for
* the loader
*/
- if (loader_len > 0) {
- /* we might not always have a NULL at the end */
- loader_str = AllocatePool(loader_len + 2);
- if (!loader_str) {
- perror(L"Failed to allocate loader string\n");
- return EFI_OUT_OF_RESOURCES;
- }
-
- for (i = 0; i < loader_len / 2; i++)
- loader_str[i] = start[i];
- loader_str[loader_len/2] = L'\0';
-
+ if (loader_str) {
second_stage = loader_str;
- load_options = remaining_size ? start + (loader_len/2) : NULL;
+ load_options = remaining;
load_options_size = remaining_size;
}
@@ -1777,12 +1763,6 @@ shim_fini(void)
unhook_exit();
- /*
- * Free the space allocated for the alternative 2nd stage loader
- */
- if (l