Verified Commit 48649287 authored by Sherif Nagy's avatar Sherif Nagy
Browse files

Adding more patches based on review board feedback...

Adding more patches based on review board feedback https://github.com/rhboot/shim-review/issues/194#issuecomment-894187000
parent 25edab96
......@@ -58,6 +58,30 @@ add {
file: "ROCKY/_supporting/0010-shim-another-attempt-to-fix-load-options-handling.patch"
}
add {
file: "ROCKY/_supporting/365.patch"
}
add {
file: "ROCKY/_supporting/PR393-1.patch"
}
add {
file: "ROCKY/_supporting/PR393-2.patch"
}
add {
file: "ROCKY/_supporting/PR396.patch"
}
add {
file: "ROCKY/_supporting/PR399-1.patch"
}
add {
file: "ROCKY/_supporting/PR399-2.patch"
}
patch {
file: "ROCKY/_supporting/0001-Adding-rocky-linux-CA.patch"
......@@ -73,7 +97,7 @@ spec_change {
append {
field: "Release"
value: ".1.2"
value: ".1.3"
}
file {
......@@ -142,6 +166,41 @@ spec_change {
add: true
}
file {
name: "365.patch"
type: Patch
add: true
}
file {
name: "PR393-1.patch"
type: Patch
add: true
}
file {
name: "PR393-2.patch"
type: Patch
add: true
}
file {
name: "PR396.patch"
type: Patch
add: true
}
file {
name: "PR399-1.patch"
type: Patch
add: true
}
file {
name: "PR399-2.patch"
type: Patch
add: true
}
file {
name: "redhatsecurebootca5.cer"
type: Source
......@@ -154,6 +213,12 @@ spec_change {
delete: true
}
changelog {
author_name: "Sherif Nagy"
author_email: "sherif@rockylinux.org"
message: "Adding more patches based on review board feedback https://github.com/rhboot/shim-review/issues/194#issuecomment-894187000"
}
changelog {
author_name: "Sherif Nagy"
author_email: "sherif@rockylinux.org"
......
From 764021ad8e01f5f5122612059ba5d8ab10ff6a3b Mon Sep 17 00:00:00 2001
From: Jonathan Yong <jonathan.yong@intel.com>
Date: Fri, 16 Apr 2021 09:59:03 +0800
Subject: [PATCH] mok: fix potential buffer overrun in import_mok_state
Fix the case where data_size is 0, so config_template is
not implicitly copied like the size calculation above.
upstream-status: https://github.com/rhboot/shim/issues/249
Signed-off-by: Jonathan Yong <jonathan.yong@intel.com>
---
mok.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/mok.c b/mok.c
index beac0ff6..db18093d 100644
--- a/mok.c
+++ b/mok.c
@@ -1034,10 +1034,12 @@ EFI_STATUS import_mok_state(EFI_HANDLE image_handle)
config_template.data_size = v->data_size;
- CopyMem(p, &config_template, sizeof(config_template));
- p += sizeof(config_template);
- CopyMem(p, v->data, v->data_size);
- p += v->data_size;
+ if (v->data && v->data_size) {
+ CopyMem(p, &config_template, sizeof(config_template));
+ p += sizeof(config_template);
+ CopyMem(p, v->data, v->data_size);
+ p += v->data_size;
+ }
}
if (p) {
ZeroMem(&config_template, sizeof(config_template));
From 25f4dcd2c7d3a989ef355e287f1feaca66fd33dd Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Mon, 26 Jul 2021 09:38:28 +0200
Subject: [PATCH] Fallback to default loader if parsed one does not exist
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
---
shim.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/shim.c b/shim.c
index 94a51768..658b0a10 100644
--- a/shim.c
+++ b/shim.c
@@ -1136,6 +1136,19 @@ EFI_STATUS init_grub(EFI_HANDLE image_handle)
use_fb ? FALLBACK : second_stage);
}
+ // If the filename is invalid, or the file does not exist,
+ // just fallback to the default loader.
+ if (!use_fb && (efi_status == EFI_INVALID_PARAMETER ||
+ efi_status == EFI_NOT_FOUND)) {
+ console_print(
+ L"start_image() returned %r, falling back to default loader\n",
+ efi_status);
+ msleep(2000000);
+ load_options = NULL;
+ load_options_size = 0;
+ efi_status = start_image(image_handle, DEFAULT_LOADER);
+ }
+
if (EFI_ERROR(efi_status)) {
console_print(L"start_image() returned %r\n", efi_status);
msleep(2000000);
From 2a76130632fba8e5f96e9d26551b6d985abf5a16 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Fri, 30 Jul 2021 12:48:36 +0200
Subject: [PATCH] shim: Dump load options in verbose mode
Dump the load options before parsing them so that we can
see which things are failing to parse.
---
include/hexdump.h | 8 ++++++++
load-options.c | 3 +++
2 files changed, 11 insertions(+)
diff --git a/include/hexdump.h b/include/hexdump.h
index 381e1a685..d1554ebd0 100644
--- a/include/hexdump.h
+++ b/include/hexdump.h
@@ -137,6 +137,7 @@ hexdumpat(const char *file, int line, const char *func, const void *data, unsign
hexdumpf(file, line, func, L"", data, size, at);
}
+#ifndef SHIM_UNIT_TEST
#define LogHexdump(data, sz) LogHexdump_(__FILE__, __LINE__, __func__, data, sz)
#define dhexdump(data, sz) hexdump(__FILE__, __LINE__, __func__, data, sz)
#define dhexdumpat(data, sz, at) \
@@ -144,5 +145,12 @@ hexdumpat(const char *file, int line, const char *func, const void *data, unsign
#define dhexdumpf(fmt, data, sz, at, ...) \
hexdumpf(__FILE__, __LINE__ - 1, __func__, fmt, data, sz, at, ##__VA_ARGS__)
+#else
+#define LogHexdump(data, sz)
+#define dhexdump(data, sz)
+#define dhexdumpat(data, sz, at)
+#define dhexdumpf(fmt, data, sz, at, ...)
+#endif
+
#endif /* STATIC_HEXDUMP_H */
// vim:fenc=utf-8:tw=75:noet
diff --git a/load-options.c b/load-options.c
index e34aa3811..c6bb74276 100644
--- a/load-options.c
+++ b/load-options.c
@@ -310,6 +310,9 @@ parse_load_options(EFI_LOADED_IMAGE *li)
UINT32 remaining_size;
CHAR16 *loader_str = NULL;
+ dprint(L"full load options:\n");
+ dhexdumpat(li->LoadOptions, li->LoadOptionsSize, 0);
+
/*
* Sanity check since we make several assumptions about the length
* Some firmware feeds the following load option when booting from
From 21aef3fa9b9d0a4ab3c683afc28565924c424a85 Mon Sep 17 00:00:00 2001
From: Jan Setje-Eilers <jan.setjeeilers@oracle.com>
Date: Mon, 26 Jul 2021 20:24:13 -0700
Subject: [PATCH] fallback: find_boot_option() needs to return the index for
the boot entry in optnum
The CopyMem() calls in add_to_boot_list() expect that
find_boot_option() returned an index to the matching entry in the
BootOrder array. The previous code returned the numerical portion of
the boot entry label, which in some cases resulted in -1 *
sizeof(CHAR16) being passed to CopyMem() which would in turn corrupt
the running firmware resulting in an exception and a failure to boot
or reset.
---
fallback.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/fallback.c b/fallback.c
index 8d89917a5..87fc3c80e 100644
--- a/fallback.c
+++ b/fallback.c
@@ -462,10 +462,15 @@ find_boot_option(EFI_DEVICE_PATH *dp, EFI_DEVICE_PATH *fulldp,
first_new_option_size = StrLen(arguments) * sizeof (CHAR16);
}
- *optnum = xtoi(varname + 4);
- FreePool(candidate);
- FreePool(data);
- return EFI_SUCCESS;
+ /* find the index for the matching entry in BootOrder */
+ UINT16 bootnum = xtoi(varname + 4);
+ for (*optnum = 0; *optnum < nbootorder; (*optnum)++) {
+ if (bootorder[*optnum] == bootnum) {
+ FreePool(candidate);
+ FreePool(data);
+ return EFI_SUCCESS;
+ }
+ }
}
FreePool(candidate);
FreePool(data);
From 6883130d13a2fbeda06289affa97b49283a3cfd7 Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Wed, 4 Aug 2021 10:43:30 +0200
Subject: [PATCH] Extract is_removable_media_path() out of
should_use_fallback()
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.
---
shim.c | 46 ++++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/shim.c b/shim.c
index 94a51768..febd2706 100644
--- a/shim.c
+++ b/shim.c
@@ -710,25 +710,12 @@ verify_buffer (char *data, int datasize,
}
static int
-should_use_fallback(EFI_HANDLE image_handle)
+is_removable_media_path(EFI_LOADED_IMAGE *li)
{
- EFI_LOADED_IMAGE *li;
unsigned int pathlen = 0;
CHAR16 *bootpath = NULL;
- EFI_FILE_IO_INTERFACE *fio = NULL;
- EFI_FILE *vh = NULL;
- EFI_FILE *fh = NULL;
- EFI_STATUS efi_status;
int ret = 0;
- efi_status = gBS->HandleProtocol(image_handle, &EFI_LOADED_IMAGE_GUID,
- (void **)&li);
- if (EFI_ERROR(efi_status)) {
- perror(L"Could not get image for bootx64.efi: %r\n",
- efi_status);
- return 0;
- }
-
bootpath = DevicePathToStr(li->FilePath);
/* Check the beginning of the string and the end, to avoid
@@ -746,6 +733,35 @@ should_use_fallback(EFI_HANDLE image_handle)
if (pathlen < 5 || StrCaseCmp(bootpath + pathlen - 4, L".EFI"))
goto error;
+ ret = 1;
+
+error:
+ if (bootpath)
+ FreePool(bootpath);
+
+ return ret;
+}
+static int
+should_use_fallback(EFI_HANDLE image_handle)
+{
+ EFI_LOADED_IMAGE *li;
+ EFI_FILE_IO_INTERFACE *fio = NULL;
+ EFI_FILE *vh = NULL;
+ EFI_FILE *fh = NULL;
+ EFI_STATUS efi_status;
+ int ret = 0;
+
+ efi_status = gBS->HandleProtocol(image_handle, &EFI_LOADED_IMAGE_GUID,
+ (void **)&li);
+ if (EFI_ERROR(efi_status)) {
+ perror(L"Could not get image for bootx64.efi: %r\n",
+ efi_status);
+ return 0;
+ }
+
+ if (!is_removable_media_path(li))
+ goto error;
+
efi_status = gBS->HandleProtocol(li->DeviceHandle, &FileSystemProtocol,
(void **) &fio);
if (EFI_ERROR(efi_status)) {
@@ -778,8 +794,6 @@ should_use_fallback(EFI_HANDLE image_handle)
fh->Close(fh);
if (vh)
vh->Close(vh);
- if (bootpath)
- FreePool(bootpath);
return ret;
}
From 062663b7ebc91df5b9e35a44351d7c3c86f3439f Mon Sep 17 00:00:00 2001
From: Julian Andres Klode <julian.klode@canonical.com>
Date: Wed, 4 Aug 2021 10:46:45 +0200
Subject: [PATCH] shim: Don't parse load options if invoked from removable
media path
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.
---
BUILDING | 6 ++++++
Make.defaults | 4 ++++
shim.c | 11 +++++++++++
3 files changed, 21 insertions(+)
diff --git a/BUILDING b/BUILDING
index ff1390fa9..2da9b5f04 100644
--- a/BUILDING
+++ b/BUILDING
@@ -45,6 +45,12 @@ Variables you could set to customize the build:
shim has already verified the kernel when shim loaded the kernel as the
second stage loader. In such a case, and only in this case, you should
use DISABLE_EBS_PROTECTION=y to build.
+- DISABLE_REMOVABLE_LOAD_OPTIONS
+ Do not parse load options when invoked as boot*.efi. This prevents boot
+ failures because of unexpected data in boot entries automatically generated
+ by firmware. It breaks loading non-default second-stage loaders when invoked
+ via that path, and requires using a binary named shim*.efi (or really anything
+ else).
- REQUIRE_TPM
if tpm logging or extends return an error code, treat that as a fatal error.
- ARCH
diff --git a/Make.defaults b/Make.defaults
index 1b929a710..af7a4cd3c 100644
--- a/Make.defaults
+++ b/Make.defaults
@@ -148,6 +148,10 @@ ifneq ($(origin DISABLE_EBS_PROTECTION), undefined)
DEFINES += -DDISABLE_EBS_PROTECTION
endif
+ifneq ($(origin DISABLE_REMOVABLE_LOAD_OPTIONS), undefined)
+ DEFINES += -DDISABLE_REMOVABLE_LOAD_OPTIONS
+endif
+
LIB_GCC = $(shell $(CC) $(ARCH_CFLAGS) -print-libgcc-file-name)
EFI_LIBS = -lefi -lgnuefi --start-group Cryptlib/libcryptlib.a Cryptlib/OpenSSL/libopenssl.a --end-group $(LIB_GCC)
FORMAT ?= --target efi-app-$(ARCH)
diff --git a/shim.c b/shim.c
index febd27065..290767fb0 100644
--- a/shim.c
+++ b/shim.c
@@ -1177,6 +1177,17 @@ EFI_STATUS set_second_stage (EFI_HANDLE image_handle)
return efi_status;
}
+#if defined(DISABLE_REMOVABLE_LOAD_OPTIONS)
+ /*
+ * boot services build very strange load options, and we might misparse them,
+ * causing boot failures on removable media.
+ */
+ if (is_removable_media_path(li)) {
+ dprint("Invoked from removable media path, ignoring boot options");
+ return EFI_SUCCESS;
+ }
+#endif
+
efi_status = parse_load_options(li);
if (EFI_ERROR(efi_status)) {
perror (L"Failed to get load options: %r\n", efi_status);
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment