grub

Merge ~xnox/grub:do-not-validate-twice into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

  1. Git
  2. lp:~xnox/grub
  3. do-not-validate-twice
  4. Merge into ubuntu
Proposed by Dimitri John Ledkov
Status: Merged
Merge reported by: Julian Andres Klode
Merged at revision: f1786b635047cabdbac852094294e60547e9f4cf
Proposed branch: ~xnox/grub:do-not-validate-twice
Merge into: ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu
Diff against target: 242 lines (+228/-0)
2 files modified
debian/patches/linuxefi-do-not-validate-kernels-twice.patch (+227/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Ubuntu Core Development Team Pending
Review via email: mp+416890@code.launchpad.net

Commit message

Do not validate kernels twice

To post a comment you must log in.
Revision history for this message
Julian Andres Klode (juliank) wrote :

Rebased and merged

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/patches/linuxefi-do-not-validate-kernels-twice.patch b/debian/patches/linuxefi-do-not-validate-kernels-twice.patch
0new file mode 1006440new file mode 100644
index 0000000..e6c059f
--- /dev/null
+++ b/debian/patches/linuxefi-do-not-validate-kernels-twice.patch
@@ -0,0 +1,227 @@
1From: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
2Date: Thu, 3 Mar 2022 13:10:56 +0100
3Subject: linuxefi: do not validate kernels twice
4
5On codebases that have shim-lock-verifier built into the grub core
6(like 2.06 upstream), shim-lock-verifier is in enforcing mode when
7booted with secureboot. It means that grub_cmd_linux() command
8attempts to perform shim validate upon opening linux kernel image,
9including kernel measurement. And the verifier correctly returns file
10open error when shim validate protocol is not present or shim fails to
11validate the kernel.
12
13This makes the call to grub_linuxefi_secure_validate() redundant, but
14also harmful. As validating the kernel image twice, extends the PCRs
15with the same measurement twice. Which breaks existing sealing
16policies when upgrading from grub2.04+rhboot+sb+linuxefi to
17grub2.06+rhboot+sb+linuxefi builds. It is also incorrect to measure
18the kernel twice.
19
20This patch must not be ported to older editions of grub code bases
21that do not have verifiers framework, or it is not builtin, or
22shim-lock-verifier is an optional module.
23
24This patch is tested to ensure that unsigned kernels are not possible
25to boot in secureboot mode when shim rejects kernel, or shim protocol
26is missing, and that the measurements become stable once again. The
27above also ensures that CVE-2020-15705 is not reintroduced.
28
29This is a backport of https://github.com/rhboot/grub2/pull/97 onto
30Ubuntu packaging.
31
32Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
33---
34 grub-core/kern/arm/coreboot/coreboot.S | 2 --
35 grub-core/loader/efi/chainloader.c | 8 ++-----
36 grub-core/loader/efi/linux.c | 12 ----------
37 grub-core/loader/efi/linux_sb.c | 41 ----------------------------------
38 grub-core/loader/i386/efi/linux.c | 13 -----------
39 include/grub/efi/linux.h | 2 --
40 6 files changed, 2 insertions(+), 76 deletions(-)
41
42diff --git a/grub-core/kern/arm/coreboot/coreboot.S b/grub-core/kern/arm/coreboot/coreboot.S
43index 70998c0..13e73ac 100644
44--- a/grub-core/kern/arm/coreboot/coreboot.S
45+++ b/grub-core/kern/arm/coreboot/coreboot.S
46@@ -42,8 +42,6 @@ FUNCTION(grub_armv7_get_timer_frequency)
47 mrc p15, 0, r0, c14, c0, 0
48 bx lr
49
50-int
51-EXPORT_FUNC(grub_linuxefi_secure_validate) (void *data, grub_uint32_t size);
52 grub_err_t
53 EXPORT_FUNC(grub_efi_linux_boot) (void *kernel_address, grub_off_t offset,
54 void *kernel_param);
55diff --git a/grub-core/loader/efi/chainloader.c b/grub-core/loader/efi/chainloader.c
56index 4a85f41..c6ae2fc 100644
57--- a/grub-core/loader/efi/chainloader.c
58+++ b/grub-core/loader/efi/chainloader.c
59@@ -910,7 +910,6 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
60 grub_efi_device_path_t *dp = 0;
61 char *filename;
62 void *boot_image = 0;
63- int rc;
64
65 if (argc == 0)
66 return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
67@@ -1080,9 +1079,7 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
68 }
69 #endif
70
71- rc = grub_linuxefi_secure_validate((void *)((grub_addr_t) address), fsize);
72- grub_dprintf ("chain", "linuxefi_secure_validate: %d\n", rc);
73- if (rc > 0)
74+ if (grub_efi_get_secureboot () == GRUB_EFI_SECUREBOOT_MODE_ENABLED)
75 {
76 grub_file_close (file);
77 if (orig_dev)
78@@ -1092,7 +1089,7 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
79 grub_secureboot_chainloader_unload, 0);
80 return 0;
81 }
82- else if (rc == 0)
83+ else
84 {
85 grub_load_and_start_image(boot_image);
86 grub_file_close (file);
87@@ -1103,7 +1100,6 @@ grub_cmd_chainloader (grub_command_t cmd __attribute__ ((unused)),
88
89 return 0;
90 }
91- // -1 fall-through to fail
92
93 fail:
94 if (orig_dev)
95diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
96index 576d8cb..24abc0c 100644
97--- a/grub-core/loader/efi/linux.c
98+++ b/grub-core/loader/efi/linux.c
99@@ -433,7 +433,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
100 struct linux_arch_kernel_header lh;
101 struct grub_arm64_linux_pe_header *pe;
102 grub_err_t err;
103- int rc;
104
105 grub_dl_ref (my_mod);
106
107@@ -478,17 +477,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
108
109 grub_dprintf ("linux", "kernel @ %p\n", kernel_addr);
110
111- if (grub_efi_get_secureboot() == GRUB_EFI_SECUREBOOT_MODE_ENABLED)
112- {
113- rc = grub_linuxefi_secure_validate (kernel_addr, kernel_size);
114- if (rc <= 0)
115- {
116- grub_error (GRUB_ERR_INVALID_COMMAND,
117- N_("%s has invalid signature"), argv[0]);
118- goto fail;
119- }
120- }
121-
122 pe = (void *)((unsigned long)kernel_addr + lh.hdr_offset);
123 handover_offset = pe->opt.entry_addr;
124
125diff --git a/grub-core/loader/efi/linux_sb.c b/grub-core/loader/efi/linux_sb.c
126index a09479c..13bcb02 100644
127--- a/grub-core/loader/efi/linux_sb.c
128+++ b/grub-core/loader/efi/linux_sb.c
129@@ -25,47 +25,6 @@
130 #include <grub/efi/linux.h>
131 #include <grub/efi/sb.h>
132
133-#define SHIM_LOCK_GUID \
134- { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
135-
136-struct grub_efi_shim_lock
137-{
138- grub_efi_status_t (*verify) (void *buffer, grub_uint32_t size);
139-};
140-typedef struct grub_efi_shim_lock grub_efi_shim_lock_t;
141-
142-// Returns 1 on success, -1 on error, 0 when not available
143-int
144-grub_linuxefi_secure_validate (void *data, grub_uint32_t size)
145-{
146- grub_efi_guid_t guid = SHIM_LOCK_GUID;
147- grub_efi_shim_lock_t *shim_lock;
148- int status;
149-
150- grub_dprintf ("linuxefi", "Locating shim protocol\n");
151- shim_lock = grub_efi_locate_protocol(&guid, NULL);
152- grub_dprintf ("secureboot", "shim_lock: %p\n", shim_lock);
153- if (!shim_lock)
154- {
155- grub_dprintf ("secureboot", "shim not available\n");
156- return 0;
157- }
158-
159- grub_dprintf ("secureboot", "Asking shim to verify kernel signature\n");
160- status = shim_lock->verify (data, size);
161- grub_dprintf ("secureboot", "shim_lock->verify(): %d\n", status);
162- if (status == GRUB_EFI_SUCCESS)
163- {
164- grub_dprintf ("secureboot", "Kernel signature verification passed\n");
165- return 1;
166- }
167-
168- grub_dprintf ("secureboot", "Kernel signature verification failed (0x%lx)\n",
169- (unsigned long) status);
170-
171- return -1;
172-}
173-
174 typedef void (*handover_func) (void *, grub_efi_system_table_t *, void *);
175
176 grub_err_t
177diff --git a/grub-core/loader/i386/efi/linux.c b/grub-core/loader/i386/efi/linux.c
178index 6a90743..cbd313a 100644
179--- a/grub-core/loader/i386/efi/linux.c
180+++ b/grub-core/loader/i386/efi/linux.c
181@@ -27,7 +27,6 @@
182 #include <grub/lib/cmdline.h>
183 #include <grub/efi/efi.h>
184 #include <grub/efi/linux.h>
185-#include <grub/efi/sb.h>
186 #include <grub/safemath.h>
187
188 GRUB_MOD_LICENSE ("GPLv3+");
189@@ -170,7 +169,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
190 grub_ssize_t start, filelen;
191 void *kernel = NULL;
192 int setup_header_end_offset;
193- int rc;
194
195 grub_dl_ref (my_mod);
196
197@@ -201,17 +199,6 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
198 goto fail;
199 }
200
201- if (grub_efi_get_secureboot() == GRUB_EFI_SECUREBOOT_MODE_ENABLED)
202- {
203- rc = grub_linuxefi_secure_validate (kernel, filelen);
204- if (rc <= 0)
205- {
206- grub_error (GRUB_ERR_ACCESS_DENIED, N_("%s has invalid signature"),
207- argv[0]);
208- goto fail;
209- }
210- }
211-
212 params = grub_efi_allocate_pages_max (0x3fffffff,
213 BYTES_TO_PAGES(sizeof(*params)));
214 if (! params)
215diff --git a/include/grub/efi/linux.h b/include/grub/efi/linux.h
216index 39f5a9a..cedf20c 100644
217--- a/include/grub/efi/linux.h
218+++ b/include/grub/efi/linux.h
219@@ -22,8 +22,6 @@
220 #include <grub/err.h>
221 #include <grub/symbol.h>
222
223-int
224-EXPORT_FUNC(grub_linuxefi_secure_validate) (void *data, grub_uint32_t size);
225 grub_err_t
226 EXPORT_FUNC(grub_efi_linux_boot) (void *kernel_address, grub_off_t offset,
227 void *kernel_param);
diff --git a/debian/patches/series b/debian/patches/series
index 7942ada..3b296a2 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -121,3 +121,4 @@ ubuntu-os-prober-auto.patch
121efi-correct-struct-grub_efi_boot_services.patch121efi-correct-struct-grub_efi_boot_services.patch
122efi-implement-grub_efi_run_image.patch122efi-implement-grub_efi_run_image.patch
123fat-fix-listing-the-root-directory.patch123fat-fix-listing-the-root-directory.patch
124linuxefi-do-not-validate-kernels-twice.patch

Subscribers

People subscribed via source and target branches