Re: [PATCH 07/24] mmap: Add grub_mmap_get_lowest() and grub_mmap_get_highest()
On 9/19/24 3:23 PM, Sergii Dmytruk wrote:
Sent 0-7 and part of 11 separately taking all the comments into account:
https://lists.gnu.org/archive/html/grub-devel/2024-09/msg00194.html
Great thanks. I will look them over, thanks for doing that. This should
make them easy to get in.
Ross
Regards,
Sergii
On Tue, Sep 17, 2024 at 11:06:51AM -0700, Ross Philipson via Grub-devel wrote:
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Daniel Kiper
The functions calculate lowest and highest available RAM
addresses respectively.
Both functions are needed to calculate PMR boundaries for
Intel TXT secure launcher introduced by subsequent patches.
After discussing this we think the best course of action is to submit the
first 7 patches standalone (also see my response to patch 11 that is
coming). For this patch I would just remove the last sentence about TXT so
it is not tied to the DRTM work.
Then you can submit this set (with the couple of the changes I suggested
earlier) and we can more easily get this part in.
Thanks
Ross
Signed-off-by: Daniel Kiper
---
grub-core/mmap/mmap.c | 83 +++
include/grub/memory.h | 3 ++
2 files changed, 86 insertions(+)
diff --git a/grub-core/mmap/mmap.c b/grub-core/mmap/mmap.c
index c8c8312c5..80d6c60b8 100644
--- a/grub-core/mmap/mmap.c
+++ b/grub-core/mmap/mmap.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
GRUB_MOD_LICENSE ("GPLv3+");
@@ -343,6 +344,88 @@ grub_mmap_unregister (int handle)
#endif /* ! GRUB_MMAP_REGISTER_BY_FIRMWARE */
+typedef struct
+{
+ grub_uint64_t addr;
+ grub_uint64_t limit;
+} addr_limit_t;
+
+/* Helper for grub_mmap_get_lowest(). */
+static int
+lowest_hook (grub_uint64_t addr, grub_uint64_t size, grub_memory_type_t type,
+ void *data)
+{
+ addr_limit_t *al = data;
+ grub_uint64_t end;
+
+ if (type != GRUB_MEMORY_AVAILABLE)
+return 0;
+
+ if (grub_add (addr, size, &end))
+return 0;
+
+ if (addr >= al->limit)
+al->addr = grub_min (al->addr, addr);
+
+ if ((addr < al->limit) && (end > al->limit))
+al->addr = al->limit;
+
+ return 0;
+}
+
+/*
+ * This function calculates lowest available RAM address that is at or above
+ * the passed limit. If no RAM exists above the limit, ~0 is returned.
+ */
+grub_uint64_t
+grub_mmap_get_lowest (grub_uint64_t limit)
+{
+ addr_limit_t al = {~0, limit};
+
+ grub_mmap_iterate (lowest_hook, &al);
+
+ return al.addr;
+}
+
+/* Helper for grub_mmap_get_highest(). */
+static int
+highest_hook (grub_uint64_t addr, grub_uint64_t size, grub_memory_type_t type,
+ void *data)
+{
+ addr_limit_t *al = data;
+ grub_uint64_t end;
+
+ if (type != GRUB_MEMORY_AVAILABLE)
+return 0;
+
+ if (grub_add (addr, size, &end))
+return 0;
+
+ if (end < al->limit)
+al->addr = grub_max (al->addr, end);
+
+ if ((addr < al->limit) && (end >= al->limit))
+al->addr = al->limit;
+
+ return 0;
+}
+
+/*
+ * This function calculates highest available RAM address that is below the
+ * passed limit. Returned address is either one byte after last byte of RAM or
+ * equal to limit, whichever is lower. If no RAM exists below limit, 0 is
+ * returned.
+ */
+grub_uint64_t
+grub_mmap_get_highest (grub_uint64_t limit)
+{
+ addr_limit_t al = {0, limit};
+
+ grub_mmap_iterate (highest_hook, &al);
+
+ return al.addr;
+}
+
#define CHUNK_SIZE 0x400
struct badram_entry {
diff --git a/include/grub/memory.h b/include/grub/memory.h
index 6da114a1b..8f22f7525 100644
--- a/include/grub/memory.h
+++ b/include/grub/memory.h
@@ -69,6 +69,9 @@ void *grub_mmap_malign_and_register (grub_uint64_t align,
grub_uint64_t size,
void grub_mmap_free_and_unregister (int handle);
+extern grub_uint64_t grub_mmap_get_lowest (grub_uint64_t limit);
+extern grub_uint64_t grub_mmap_get_highest (grub_uint64_t limit);
+
#ifndef GRUB_MMAP_REGISTER_BY_FIRMWARE
struct grub_mmap_region
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 10/24] include/grub: Introduce Secure Launch Resource Table (SLRT)
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Ross Philipson
Provide definitions of structures and basic functions for constructing
and parsing of SLRT.
I looked this all over and it is inline with the layout of the tables
(ABI) of the most recent SLRT (both in the v11 release code and the
spec). I noticed you enhanced the error checking in the functions which
is helpful.
Since I was the originating author I cannot give an R-b but everything
looks good.
Ross
Signed-off-by: Ross Philipson
Signed-off-by: Sergii Dmytruk
Signed-off-by: Krystian Hebel
---
include/grub/slr_table.h | 328 +++
1 file changed, 328 insertions(+)
create mode 100644 include/grub/slr_table.h
diff --git a/include/grub/slr_table.h b/include/grub/slr_table.h
new file mode 100644
index 0..514ec5feb
--- /dev/null
+++ b/include/grub/slr_table.h
@@ -0,0 +1,328 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2023 Oracle and/or its affiliates.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see
<https://urldefense.com/v3/__http://www.gnu.org/licenses/__;!!ACWV5N9M2RV99hQ!NFbTYjZr8_0zYpDhu3moYLZsp33Dse3HKGdTgM7GC_WCdQ0iHLLAJj0dZuM-bAHBqsqHCW9LDF6V3lOHK1f8zYkxTf9O$
>.
+ *
+ * Secure Launch Resource Table definitions
+ */
+
+#ifndef GRUB_SLR_TABLE_H
+#define GRUB_SLR_TABLE_H 1
+
+#include
+
+#define GRUB_UEFI_SLR_TABLE_GUID \
+ { 0x877a9b2a, 0x0385, 0x45d1, { 0xa0, 0x34, 0x9d, 0xac, 0x9c, 0x9e, 0x56,
0x5f }}
+
+/* SLR table header values */
+#define GRUB_SLR_TABLE_MAGIC 0x4452544d
+#define GRUB_SLR_TABLE_REVISION1
+
+/* Current revisions for the policy and UEFI config */
+#define GRUB_SLR_POLICY_REVISION 1
+#define GRUB_SLR_UEFI_CONFIG_REVISION 1
+
+/* SLR defined architectures */
+#define GRUB_SLR_INTEL_TXT 1
+#define GRUB_SLR_AMD_SKINIT2
+
+/* SLR defined bootloaders */
+#define GRUB_SLR_BOOTLOADER_INVALID0
+#define GRUB_SLR_BOOTLOADER_GRUB 1
+
+/* Log formats */
+#define GRUB_SLR_DRTM_TPM12_LOG1
+#define GRUB_SLR_DRTM_TPM20_LOG2
+
+/* DRTM Policy Entry Flags */
+#define GRUB_SLR_POLICY_FLAG_MEASURED 0x1
+#define GRUB_SLR_POLICY_IMPLICIT_SIZE 0x2
+
+/* Array Lengths */
+#define GRUB_TPM_EVENT_INFO_LENGTH 32
+#define GRUB_TXT_VARIABLE_MTRRS_LENGTH 32
+
+/* Tags */
+#define GRUB_SLR_ENTRY_INVALID 0x
+#define GRUB_SLR_ENTRY_DL_INFO 0x0001
+#define GRUB_SLR_ENTRY_LOG_INFO0x0002
+#define GRUB_SLR_ENTRY_DRTM_POLICY 0x0003
+#define GRUB_SLR_ENTRY_INTEL_INFO 0x0004
+#define GRUB_SLR_ENTRY_AMD_INFO0x0005
+#define GRUB_SLR_ENTRY_ARM_INFO0x0006
+#define GRUB_SLR_ENTRY_UEFI_INFO 0x0007
+#define GRUB_SLR_ENTRY_UEFI_CONFIG 0x0008
+#define GRUB_SLR_ENTRY_END 0x
+
+/* Entity Types */
+#define GRUB_SLR_ET_UNSPECIFIED0x
+#define GRUB_SLR_ET_SLRT 0x0001
+#define GRUB_SLR_ET_BOOT_PARAMS0x0002
+#define GRUB_SLR_ET_SETUP_DATA 0x0003
+#define GRUB_SLR_ET_CMDLINE0x0004
+#define GRUB_SLR_ET_UEFI_MEMMAP0x0005
+#define GRUB_SLR_ET_RAMDISK0x0006
+#define GRUB_SLR_ET_MULTIBOOT2_INFO0x0007
+#define GRUB_SLR_ET_MULTIBOOT2_MODULE 0x0008
+#define GRUB_SLR_ET_TXT_OS2MLE 0x0010
+#define GRUB_SLR_ET_UNUSED 0x
+
+/*
+ * Primary Secure Launch Resource Table Header
+ */
+struct grub_slr_table
+{
+ grub_uint32_t magic;
+ grub_uint16_t revision;
+ grub_uint16_t architecture;
+ grub_uint32_t size;
+ grub_uint32_t max_size;
+ /* table entries */
+} GRUB_PACKED;
+
+/*
+ * Common SLRT Table Header
+ */
+struct grub_slr_entry_hdr
+{
+ grub_uint32_t tag;
+ grub_uint32_t size;
+} GRUB_PACKED;
+
+/*
+ * Boot loader context
+ */
+struct grub_slr_bl_context
+{
+ grub_uint16_t bootloader;
+ grub_uint16_t reserved[3];
+ grub_uint64_t context;
+} GRUB_PACKED;
+
+/*
+ * Dynamic Launch Callback Function type
+ */
+typedef void (*grub_dl_handler_func)(struct grub_slr_bl_context *bl_context);
+
+/*
+ * DRTM Dynamic Launch Configuration
+ */
+struct grub_slr_entry_dl_info
+{
+ struct grub_slr_entry_hdr hdr;
+ grub_uint64_t dce_size;
+ grub_uint64_t dce_base;
+ grub_uint64_t dlme_size;
+ grub_uint64_t dlm
Re: [PATCH 11/24] i386/slaunch: Add basic platform support for secure launch
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Ross Philipson
Some of the commands declared in header files will be implemented in
the follow-up commits.
Oh and in the split out patch, can you fix the commit message to just
indicate this is generic x86 bits?
Ross
Signed-off-by: Ross Philipson
Signed-off-by: Daniel Kiper
Signed-off-by: Krystian Hebel
Signed-off-by: Sergii Dmytruk
---
include/grub/i386/cpuid.h | 12
include/grub/i386/crfr.h| 127
include/grub/i386/mmio.h| 72
include/grub/i386/msr.h | 63 ++
include/grub/i386/slaunch.h | 77 ++
5 files changed, 351 insertions(+)
create mode 100644 include/grub/i386/crfr.h
create mode 100644 include/grub/i386/mmio.h
create mode 100644 include/grub/i386/slaunch.h
diff --git a/include/grub/i386/cpuid.h b/include/grub/i386/cpuid.h
index f7ae4b0a4..0ddd87b15 100644
--- a/include/grub/i386/cpuid.h
+++ b/include/grub/i386/cpuid.h
@@ -19,6 +19,18 @@
#ifndef GRUB_CPU_CPUID_HEADER
#define GRUB_CPU_CPUID_HEADER 1
+/* General */
+#define GRUB_X86_CPUID_VENDOR 0x
+#define GRUB_X86_CPUID_FEATURES0x0001
+/* Intel */
+#define GRUB_X86_CPUID_FEATURES_ECX_VMX(1<<5)
+#define GRUB_X86_CPUID_FEATURES_ECX_SMX(1<<6)
+
+/* AMD */
+#define GRUB_AMD_CPUID_FEATURES0x8001
+#define GRUB_AMD_CPUID_FEATURES_ECX_SVM(1<<2)
+#define GRUB_AMD_CPUID_FUNC0x800a
+
extern unsigned char grub_cpuid_has_longmode;
extern unsigned char grub_cpuid_has_pae;
diff --git a/include/grub/i386/crfr.h b/include/grub/i386/crfr.h
new file mode 100644
index 0..881a4dad4
--- /dev/null
+++ b/include/grub/i386/crfr.h
@@ -0,0 +1,127 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2020 Oracle and/or its affiliates.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see
<https://urldefense.com/v3/__https://www.gnu.org/licenses/__;!!ACWV5N9M2RV99hQ!NqURlAB94pWy-U_y0OQCEskDAeDx649zRrId6IAY4aLN-cTVUL83BqeuRNwg2voFzfxGaH1xrK7OGQO5jMzkl3v4DIDY$
>.
+ */
+
+#ifndef GRUB_CRFR_H
+#define GRUB_CRFR_H 1
+
+#include
+
+/* Routines for R/W of control and flags registers */
+
+#define GRUB_CR0_X86_PE0x0001 /* Enable Protected Mode */
+#define GRUB_CR0_X86_MP0x0002 /* "Math" (FPU) Present */
+#define GRUB_CR0_X86_EM0x0004 /* EMulate FPU */
+#define GRUB_CR0_X86_TS0x0008 /* Task Switched */
+#define GRUB_CR0_X86_PG0x8000 /* Enable PaGing */
+
+#define GRUB_CR0_X86_NE0x0020 /* Numeric Error enable
(EX16 vs IRQ13) */
+#define GRUB_CR0_X86_WP0x0001 /* Write Protect */
+#define GRUB_CR0_X86_AM0x0004 /* Alignment Mask */
+#define GRUB_CR0_X86_NW0x2000 /* Not Write-through */
+#define GRUB_CR0_X86_CD0x4000 /* Cache Disable */
+
+#define GRUB_CR4_X86_VME 0x0001 /* Virtual 8086 mode extensions */
+#define GRUB_CR4_X86_PVI 0x0002 /* Protected-mode virtual interrupts
*/
+#define GRUB_CR4_X86_TSD 0x0004 /* Time stamp disable */
+#define GRUB_CR4_X86_DE0x0008 /* Debugging extensions */
+#define GRUB_CR4_X86_PSE 0x0010 /* Page size extensions */
+#define GRUB_CR4_X86_PAE 0x0020 /* Physical address extension */
+#define GRUB_CR4_X86_MCE 0x0040 /* Enable Machine check enable */
+#define GRUB_CR4_X86_PGE 0x0080 /* Enable Page global */
+#define GRUB_CR4_X86_PCE 0x0100 /* Enable Performance monitoring
counter */
+#define GRUB_CR4_X86_FXSR 0x0200 /* Fast FPU save/restore */
+#define GRUB_CR4_X86_XMM 0x0400 /* Generate #XM instead of #UD for
SIMD */
+#define GRUB_CR4_X86_VMXE 0x2000 /* Enable VMX */
+#define GRUB_CR4_X86_SMXE 0x4000 /* Enable SMX */
+#define GRUB_CR4_X86_PCIDE 0x0002 /* Enable PCID */
+
+static inline unsigned long
+grub_read_cr0 (void)
+{
+ unsigned long val;
+
+ asm volatile ("mov %%cr0, %0" : "=r" (val) : : "memory");
+
+ return val;
+}
+
+static inline void
+grub_write_cr0 (unsigned long val)
+{
+ asm volatile ("mov %0, %%cr0" : : "r" (val) : "memory&quo
Re: [PATCH 11/24] i386/slaunch: Add basic platform support for secure launch
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Ross Philipson
Some of the commands declared in header files will be implemented in
the follow-up commits.
In addition to submitting the first seven patches standalone, if you
split this patch up and remove the slaunch.h changes, you can then
submit the rest of this patch (which is just generic x86 stuff) with the
other other seven. Does this make sense?
Thanks
Ross
Signed-off-by: Ross Philipson
Signed-off-by: Daniel Kiper
Signed-off-by: Krystian Hebel
Signed-off-by: Sergii Dmytruk
---
include/grub/i386/cpuid.h | 12
include/grub/i386/crfr.h| 127
include/grub/i386/mmio.h| 72
include/grub/i386/msr.h | 63 ++
include/grub/i386/slaunch.h | 77 ++
5 files changed, 351 insertions(+)
create mode 100644 include/grub/i386/crfr.h
create mode 100644 include/grub/i386/mmio.h
create mode 100644 include/grub/i386/slaunch.h
diff --git a/include/grub/i386/cpuid.h b/include/grub/i386/cpuid.h
index f7ae4b0a4..0ddd87b15 100644
--- a/include/grub/i386/cpuid.h
+++ b/include/grub/i386/cpuid.h
@@ -19,6 +19,18 @@
#ifndef GRUB_CPU_CPUID_HEADER
#define GRUB_CPU_CPUID_HEADER 1
+/* General */
+#define GRUB_X86_CPUID_VENDOR 0x
+#define GRUB_X86_CPUID_FEATURES0x0001
+/* Intel */
+#define GRUB_X86_CPUID_FEATURES_ECX_VMX(1<<5)
+#define GRUB_X86_CPUID_FEATURES_ECX_SMX(1<<6)
+
+/* AMD */
+#define GRUB_AMD_CPUID_FEATURES0x8001
+#define GRUB_AMD_CPUID_FEATURES_ECX_SVM(1<<2)
+#define GRUB_AMD_CPUID_FUNC0x800a
+
extern unsigned char grub_cpuid_has_longmode;
extern unsigned char grub_cpuid_has_pae;
diff --git a/include/grub/i386/crfr.h b/include/grub/i386/crfr.h
new file mode 100644
index 0..881a4dad4
--- /dev/null
+++ b/include/grub/i386/crfr.h
@@ -0,0 +1,127 @@
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2020 Oracle and/or its affiliates.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see
<https://urldefense.com/v3/__https://www.gnu.org/licenses/__;!!ACWV5N9M2RV99hQ!NqURlAB94pWy-U_y0OQCEskDAeDx649zRrId6IAY4aLN-cTVUL83BqeuRNwg2voFzfxGaH1xrK7OGQO5jMzkl3v4DIDY$
>.
+ */
+
+#ifndef GRUB_CRFR_H
+#define GRUB_CRFR_H 1
+
+#include
+
+/* Routines for R/W of control and flags registers */
+
+#define GRUB_CR0_X86_PE0x0001 /* Enable Protected Mode */
+#define GRUB_CR0_X86_MP0x0002 /* "Math" (FPU) Present */
+#define GRUB_CR0_X86_EM0x0004 /* EMulate FPU */
+#define GRUB_CR0_X86_TS0x0008 /* Task Switched */
+#define GRUB_CR0_X86_PG0x8000 /* Enable PaGing */
+
+#define GRUB_CR0_X86_NE0x0020 /* Numeric Error enable
(EX16 vs IRQ13) */
+#define GRUB_CR0_X86_WP0x0001 /* Write Protect */
+#define GRUB_CR0_X86_AM0x0004 /* Alignment Mask */
+#define GRUB_CR0_X86_NW0x2000 /* Not Write-through */
+#define GRUB_CR0_X86_CD0x4000 /* Cache Disable */
+
+#define GRUB_CR4_X86_VME 0x0001 /* Virtual 8086 mode extensions */
+#define GRUB_CR4_X86_PVI 0x0002 /* Protected-mode virtual interrupts
*/
+#define GRUB_CR4_X86_TSD 0x0004 /* Time stamp disable */
+#define GRUB_CR4_X86_DE0x0008 /* Debugging extensions */
+#define GRUB_CR4_X86_PSE 0x0010 /* Page size extensions */
+#define GRUB_CR4_X86_PAE 0x0020 /* Physical address extension */
+#define GRUB_CR4_X86_MCE 0x0040 /* Enable Machine check enable */
+#define GRUB_CR4_X86_PGE 0x0080 /* Enable Page global */
+#define GRUB_CR4_X86_PCE 0x0100 /* Enable Performance monitoring
counter */
+#define GRUB_CR4_X86_FXSR 0x0200 /* Fast FPU save/restore */
+#define GRUB_CR4_X86_XMM 0x0400 /* Generate #XM instead of #UD for
SIMD */
+#define GRUB_CR4_X86_VMXE 0x2000 /* Enable VMX */
+#define GRUB_CR4_X86_SMXE 0x4000 /* Enable SMX */
+#define GRUB_CR4_X86_PCIDE 0x0002 /* Enable PCID */
+
+static inline unsigned long
+grub_read_cr0 (void)
+{
+ unsigned long val;
+
+ asm volatile ("mov %%cr0, %0" : "=r" (val) : : "memory");
+
+ return
Re: [PATCH 07/24] mmap: Add grub_mmap_get_lowest() and grub_mmap_get_highest()
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Daniel Kiper
The functions calculate lowest and highest available RAM
addresses respectively.
Both functions are needed to calculate PMR boundaries for
Intel TXT secure launcher introduced by subsequent patches.
After discussing this we think the best course of action is to submit
the first 7 patches standalone (also see my response to patch 11 that is
coming). For this patch I would just remove the last sentence about TXT
so it is not tied to the DRTM work.
Then you can submit this set (with the couple of the changes I suggested
earlier) and we can more easily get this part in.
Thanks
Ross
Signed-off-by: Daniel Kiper
---
grub-core/mmap/mmap.c | 83 +++
include/grub/memory.h | 3 ++
2 files changed, 86 insertions(+)
diff --git a/grub-core/mmap/mmap.c b/grub-core/mmap/mmap.c
index c8c8312c5..80d6c60b8 100644
--- a/grub-core/mmap/mmap.c
+++ b/grub-core/mmap/mmap.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
GRUB_MOD_LICENSE ("GPLv3+");
@@ -343,6 +344,88 @@ grub_mmap_unregister (int handle)
#endif /* ! GRUB_MMAP_REGISTER_BY_FIRMWARE */
+typedef struct
+{
+ grub_uint64_t addr;
+ grub_uint64_t limit;
+} addr_limit_t;
+
+/* Helper for grub_mmap_get_lowest(). */
+static int
+lowest_hook (grub_uint64_t addr, grub_uint64_t size, grub_memory_type_t type,
+ void *data)
+{
+ addr_limit_t *al = data;
+ grub_uint64_t end;
+
+ if (type != GRUB_MEMORY_AVAILABLE)
+return 0;
+
+ if (grub_add (addr, size, &end))
+return 0;
+
+ if (addr >= al->limit)
+al->addr = grub_min (al->addr, addr);
+
+ if ((addr < al->limit) && (end > al->limit))
+al->addr = al->limit;
+
+ return 0;
+}
+
+/*
+ * This function calculates lowest available RAM address that is at or above
+ * the passed limit. If no RAM exists above the limit, ~0 is returned.
+ */
+grub_uint64_t
+grub_mmap_get_lowest (grub_uint64_t limit)
+{
+ addr_limit_t al = {~0, limit};
+
+ grub_mmap_iterate (lowest_hook, &al);
+
+ return al.addr;
+}
+
+/* Helper for grub_mmap_get_highest(). */
+static int
+highest_hook (grub_uint64_t addr, grub_uint64_t size, grub_memory_type_t type,
+ void *data)
+{
+ addr_limit_t *al = data;
+ grub_uint64_t end;
+
+ if (type != GRUB_MEMORY_AVAILABLE)
+return 0;
+
+ if (grub_add (addr, size, &end))
+return 0;
+
+ if (end < al->limit)
+al->addr = grub_max (al->addr, end);
+
+ if ((addr < al->limit) && (end >= al->limit))
+al->addr = al->limit;
+
+ return 0;
+}
+
+/*
+ * This function calculates highest available RAM address that is below the
+ * passed limit. Returned address is either one byte after last byte of RAM or
+ * equal to limit, whichever is lower. If no RAM exists below limit, 0 is
+ * returned.
+ */
+grub_uint64_t
+grub_mmap_get_highest (grub_uint64_t limit)
+{
+ addr_limit_t al = {0, limit};
+
+ grub_mmap_iterate (highest_hook, &al);
+
+ return al.addr;
+}
+
#define CHUNK_SIZE0x400
struct badram_entry {
diff --git a/include/grub/memory.h b/include/grub/memory.h
index 6da114a1b..8f22f7525 100644
--- a/include/grub/memory.h
+++ b/include/grub/memory.h
@@ -69,6 +69,9 @@ void *grub_mmap_malign_and_register (grub_uint64_t align,
grub_uint64_t size,
void grub_mmap_free_and_unregister (int handle);
+extern grub_uint64_t grub_mmap_get_lowest (grub_uint64_t limit);
+extern grub_uint64_t grub_mmap_get_highest (grub_uint64_t limit);
+
#ifndef GRUB_MMAP_REGISTER_BY_FIRMWARE
struct grub_mmap_region
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 04/24] i386/memory: Rename PAGE_SHIFT to GRUB_PAGE_SHIFT
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Daniel Kiper
...to avoid potential conflicts and confusion.
Please fix these commit messages to be complete and not use the ellipsis
to reference the short description.
Thanks
Ross
Signed-off-by: Daniel Kiper
---
grub-core/lib/i386/xen/relocator.S | 6 +++---
grub-core/lib/x86_64/xen/relocator.S | 4 ++--
grub-core/loader/i386/xen.c | 28 ++--
include/grub/i386/memory.h | 2 +-
4 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/grub-core/lib/i386/xen/relocator.S
b/grub-core/lib/i386/xen/relocator.S
index 96e51b59a..dab4d8ace 100644
--- a/grub-core/lib/i386/xen/relocator.S
+++ b/grub-core/lib/i386/xen/relocator.S
@@ -75,10 +75,10 @@ VARIABLE(grub_relocator_xen_mfn_list)
.long 0
movl0(%eax, %ebp, 4), %ecx /* mfn */
movl%ebp, %ebx
- shll$PAGE_SHIFT, %ebx /* virtual address (1:1 mapping) */
+ shll$GRUB_PAGE_SHIFT, %ebx /* virtual address (1:1 mapping) */
movl%ecx, %edx
- shll$PAGE_SHIFT, %ecx /* prepare pte low part */
- shrl$(32 - PAGE_SHIFT), %edx /* pte high part */
+ shll$GRUB_PAGE_SHIFT, %ecx /* prepare pte low part */
+ shrl$(32 - GRUB_PAGE_SHIFT), %edx /* pte high part */
orl $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %ecx /* pte low */
movl$UVMF_INVLPG, %esi
movl$__HYPERVISOR_update_va_mapping, %eax
diff --git a/grub-core/lib/x86_64/xen/relocator.S
b/grub-core/lib/x86_64/xen/relocator.S
index f5364ed0f..852cd40aa 100644
--- a/grub-core/lib/x86_64/xen/relocator.S
+++ b/grub-core/lib/x86_64/xen/relocator.S
@@ -60,9 +60,9 @@ LOCAL(cont):
jz 3f
2:
movq%r12, %rdi
- shlq$PAGE_SHIFT, %rdi /* virtual address (1:1 mapping) */
+ shlq$GRUB_PAGE_SHIFT, %rdi /* virtual address (1:1 mapping) */
movq(%rbx, %r12, 8), %rsi /* mfn */
- shlq$PAGE_SHIFT, %rsi
+ shlq$GRUB_PAGE_SHIFT, %rsi
orq $(GRUB_PAGE_PRESENT | GRUB_PAGE_USER), %rsi /* Build pte */
movq$UVMF_INVLPG, %rdx
movq%rcx, %r9 /* %rcx clobbered by hypercall */
diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
index 3b856e842..520367732 100644
--- a/grub-core/loader/i386/xen.c
+++ b/grub-core/loader/i386/xen.c
@@ -92,7 +92,7 @@ static struct xen_loader_state xen_state;
static grub_dl_t my_mod;
-#define PAGE_SIZE (1UL << PAGE_SHIFT)
+#define PAGE_SIZE (1UL << GRUB_PAGE_SHIFT)
#define MAX_MODULES (PAGE_SIZE / sizeof (struct xen_multiboot_mod_list))
#define STACK_SIZE 1048576
#define ADDITIONAL_SIZE (1 << 19)
@@ -103,7 +103,7 @@ static grub_dl_t my_mod;
static grub_uint64_t
page2offset (grub_uint64_t page)
{
- return page << PAGE_SHIFT;
+ return page << GRUB_PAGE_SHIFT;
}
static grub_err_t
@@ -142,7 +142,7 @@ get_pgtable_size (grub_uint64_t from, grub_uint64_t to,
grub_uint64_t pfn)
continue;
}
- bits = PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE;
+ bits = GRUB_PAGE_SHIFT + (i + 1) * LOG_POINTERS_PER_PAGE;
mask = (1ULL << bits) - 1;
map->lvls[i].virt_start = map->area.virt_start & ~mask;
map->lvls[i].virt_end = map->area.virt_end | mask;
@@ -247,11 +247,11 @@ generate_page_table (grub_xen_mfn_t *mfn_list)
if (lvl->virt_start >= end || lvl->virt_end <= start)
continue;
p_s = (grub_max (start, lvl->virt_start) - start) >>
- (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
+ (GRUB_PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
p_e = (grub_min (end, lvl->virt_end) - start) >>
- (PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
+ (GRUB_PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE);
pfn = ((grub_max (start, lvl->virt_start) - lvl->virt_start) >>
-(PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE)) + lvl->pfn_start;
+(GRUB_PAGE_SHIFT + l * LOG_POINTERS_PER_PAGE)) +
lvl->pfn_start;
grub_dprintf ("xen", "write page table entries level %d pg %p "
"mapping %d/%d index %lx-%lx pfn %llx\n",
l, pg, m1, m2, p_s, p_e, (unsigned long long) pfn);
@@ -329,16 +329,16 @@ grub_xen_p2m_alloc (void)
{
err = get_pgtable_size (xen_state.xen_inf.p2m_base,
xen_state.xen_inf.p2m_base + p2msize,
- (xen_state.max_addr + p2msize) >> PAGE_SHIFT);
+ (xen_state.max_addr + p2msize) >>
GRUB_PAGE_SHIFT);
if (err)
return err;
- map->area.pfn_start = xen_state.max_addr >> PAGE_SHIFT;
+ map->area.pfn_start = xen_state.max_addr >> GRUB_PAGE_SHIFT;
p2malloc = p2msize + page2offset (map->area.n_pt_pages);
xen_state.n_m
Re: [PATCH 03/24] i386/msr: Extract and improve MSR support detection code
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Daniel Kiper
Currently rdmsr and wrmsr commands have own MSR support detection code.
This code is the same. So, it is duplicated. Additionally, this code
cannot be reused by others. Hence, extract this code to a function and
make it public. By the way, improve a code a bit.
Additionally, use GRUB_ERR_BAD_DEVICE instead of GRUB_ERR_BUG to signal
an error because errors encountered by this new routine are not bugs.
Same here - you should add an SoB line for yourself. This applies to
some of the other submitted patches also.
Ross
Signed-off-by: Daniel Kiper
---
grub-core/commands/i386/rdmsr.c | 21 +
grub-core/commands/i386/wrmsr.c | 21 +
include/grub/i386/msr.h | 29 +
3 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
index 89ece7657..2e42f6197 100644
--- a/grub-core/commands/i386/rdmsr.c
+++ b/grub-core/commands/i386/rdmsr.c
@@ -42,27 +42,16 @@ static const struct grub_arg_option options[] =
static grub_err_t
grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
{
- grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
+ grub_err_t err;
+ grub_uint32_t addr;
grub_uint64_t value;
const char *ptr;
char buf[sizeof("1122334455667788")];
- /*
- * The CPUID instruction should be used to determine whether MSRs
- * are supported. (CPUID.01H:EDX[5] = 1)
- */
- if (! grub_cpu_is_cpuid_supported ())
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+ err = grub_cpu_is_msr_supported ();
- grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
-
- if (max_cpuid < 1)
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
-
- grub_cpuid (1, a, b, c, features);
-
- if (!(features & (1 << 5)))
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+ if (err != GRUB_ERR_NONE)
+return grub_error (err, N_("RDMSR is unsupported"));
if (argc != 1)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument expected"));
diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
index cf6bf6c8f..7fbedaed9 100644
--- a/grub-core/commands/i386/wrmsr.c
+++ b/grub-core/commands/i386/wrmsr.c
@@ -36,26 +36,15 @@ static grub_command_t cmd_write;
static grub_err_t
grub_cmd_msr_write (grub_command_t cmd __attribute__ ((unused)), int argc,
char **argv)
{
- grub_uint32_t manufacturer[3], max_cpuid, a, b, c, features, addr;
+ grub_err_t err;
+ grub_uint32_t addr;
grub_uint64_t value;
const char *ptr;
- /*
- * The CPUID instruction should be used to determine whether MSRs
- * are supported. (CPUID.01H:EDX[5] = 1)
- */
- if (!grub_cpu_is_cpuid_supported ())
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+ err = grub_cpu_is_msr_supported ();
- grub_cpuid (0, max_cpuid, manufacturer[0], manufacturer[2], manufacturer[1]);
-
- if (max_cpuid < 1)
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
-
- grub_cpuid (1, a, b, c, features);
-
- if (!(features & (1 << 5)))
-return grub_error (GRUB_ERR_BUG, N_("unsupported instruction"));
+ if (err != GRUB_ERR_NONE)
+return grub_error (err, N_("WRMSR is unsupported"));
if (argc != 2)
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
index 4fba1b8e0..1e838c022 100644
--- a/include/grub/i386/msr.h
+++ b/include/grub/i386/msr.h
@@ -19,6 +19,35 @@
#ifndef GRUB_I386_MSR_H
#define GRUB_I386_MSR_H 1
+#include
+#include
+#include
+
+static inline grub_err_t
+grub_cpu_is_msr_supported (void)
+{
+ grub_uint32_t eax, ebx, ecx, edx;
+
+ /*
+ * The CPUID instruction should be used to determine whether MSRs
+ * are supported, CPUID.01H:EDX[5] = 1.
+ */
+ if (!grub_cpu_is_cpuid_supported ())
+return GRUB_ERR_BAD_DEVICE;
+
+ grub_cpuid (0, eax, ebx, ecx, edx);
+
+ if (eax < 1)
+return GRUB_ERR_BAD_DEVICE;
+
+ grub_cpuid (1, eax, ebx, ecx, edx);
+
+ if (!(edx & (1 << 5)))
+return GRUB_ERR_BAD_DEVICE;
+
+ return GRUB_ERR_NONE;
+}
+
/*
* TODO: Add a general protection exception handler.
* Accessing a reserved or unimplemented MSR address results in a GP#.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 02/24] i386/msr: Rename grub_msr_read() and grub_msr_write()
On 8/26/24 5:44 AM, Sergii Dmytruk wrote:
From: Daniel Kiper
... to grub_rdmsr() and grub_wrmsr() respectively. New names are more
obvious than older ones.
This patch needs its commit message fixed to remove the ellipsis and
just make it a complete sentence.
Also since you are not the author, you should add an SoB for yourself
since you are submitting it. This seems to be best practice.
The patch itself looks fine.
Thanks
Ross
Signed-off-by: Daniel Kiper
---
grub-core/commands/i386/rdmsr.c | 2 +-
grub-core/commands/i386/wrmsr.c | 2 +-
include/grub/i386/msr.h | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/grub-core/commands/i386/rdmsr.c b/grub-core/commands/i386/rdmsr.c
index fa4622f9e..89ece7657 100644
--- a/grub-core/commands/i386/rdmsr.c
+++ b/grub-core/commands/i386/rdmsr.c
@@ -76,7 +76,7 @@ grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char
**argv)
if (*ptr != '\0')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
- value = grub_msr_read (addr);
+ value = grub_rdmsr (addr);
if (ctxt->state[0].set)
{
diff --git a/grub-core/commands/i386/wrmsr.c b/grub-core/commands/i386/wrmsr.c
index 8f352f205..cf6bf6c8f 100644
--- a/grub-core/commands/i386/wrmsr.c
+++ b/grub-core/commands/i386/wrmsr.c
@@ -77,7 +77,7 @@ grub_cmd_msr_write (grub_command_t cmd __attribute__
((unused)), int argc, char
if (*ptr != '\0')
return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
- grub_msr_write (addr, value);
+ grub_wrmsr (addr, value);
return GRUB_ERR_NONE;
}
diff --git a/include/grub/i386/msr.h b/include/grub/i386/msr.h
index 7b52b5d61..4fba1b8e0 100644
--- a/include/grub/i386/msr.h
+++ b/include/grub/i386/msr.h
@@ -25,7 +25,7 @@
*/
static inline grub_uint64_t
-grub_msr_read (grub_uint32_t msr_id)
+grub_rdmsr (grub_uint32_t msr_id)
{
grub_uint32_t low, high;
@@ -35,7 +35,7 @@ grub_msr_read (grub_uint32_t msr_id)
}
static inline void
-grub_msr_write(grub_uint32_t msr_id, grub_uint64_t msr_value)
+grub_wrmsr (grub_uint32_t msr_id, grub_uint64_t msr_value)
{
grub_uint32_t low = msr_value, high = msr_value >> 32;
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] Fixes for CIDs 86750 and 396931
On 10/17/22 10:04, Jagannathan Raman wrote: Hi, This series provides fixes for CIDs 86750 and 396931. Kindly share your feedback. Thank you very much! These LGTM Reviewed-by: Ross Philipson -- Jag Jagannathan Raman (2): zfs: dnode_get_path(): update dangling dn_new pointer kern/buffer: grub_buffer_free: handle NULL input pointer grub-core/fs/zfs/zfs.c | 6 ++ grub-core/kern/buffer.c | 7 +-- 2 files changed, 11 insertions(+), 2 deletions(-) ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Initialize BSD relocator state variables
Numerous register fields in the relocator state are simply not
used depending on the relocator. This causes Coverity to flag
these fields but there is no real bug here. Simply initializing
the variable to {0} solves the issue. Fixed in the else case too
for consistency.
Fixes: CID 396932
Signed-off-by: Ross Philipson
---
grub-core/loader/i386/bsd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 5838fc8f4..1f9128f6f 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -728,7 +728,7 @@ grub_freebsd_boot (void)
if (is_64bit)
{
- struct grub_relocator64_state state;
+ struct grub_relocator64_state state = {0};
grub_uint8_t *pagetable;
grub_uint32_t *stack;
grub_addr_t stack_target;
@@ -767,7 +767,7 @@ grub_freebsd_boot (void)
}
else
{
- struct grub_relocator32_state state;
+ struct grub_relocator32_state state = {0};
grub_uint32_t *stack;
grub_addr_t stack_target;
--
2.37.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] Initialize local relocator subchunk struct to all zeros
The way the code is written the tofree variable would never be
passed to the free_subchunk() function uninitialized. Coverity
cannot determine this and flags the situation as "Using uninitialized
value...". The fix is just to initialize the local struct.
Fixes: CID 314016
Signed-off-by: Ross Philipson
---
grub-core/lib/relocator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/grub-core/lib/relocator.c b/grub-core/lib/relocator.c
index 68ef128..bfcc70d 100644
--- a/grub-core/lib/relocator.c
+++ b/grub-core/lib/relocator.c
@@ -989,7 +989,7 @@ malloc_in_range (struct grub_relocator *rel,
if (j != 0 && events[j - 1].pos != events[j].pos)
{
grub_addr_t alloc_start, alloc_end;
- struct grub_relocator_subchunk tofree;
+ struct grub_relocator_subchunk tofree = {0};
struct grub_relocator_subchunk *curschu = &tofree;
if (!oom)
curschu = &res->subchunks[cural];
--
1.8.3.1
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [GRUB PATCH RFC 00/18] i386: Intel TXT secure launcher
On 6/1/20 1:56 PM, Daniel P. Smith wrote: > On 6/1/20 12:51 PM, Andy Lutomirski wrote: >> On Mon, Jun 1, 2020 at 8:33 AM Daniel P. Smith >> wrote: >>> >>> On 5/7/20 7:06 AM, Daniel Kiper wrote: Hi Łukasz, On Tue, May 05, 2020 at 04:38:02PM +0200, Lukasz Hawrylko wrote: > On Tue, 2020-05-05 at 01:21 +0200, Daniel Kiper wrote: >>> >>> ... >>> > In OS-MLE table there is a buffer for TPM event log, however I see that > you are not using it, but instead allocate space somewhere in the I think that this part requires more discussion. In my opinion we should have this region dynamically allocated because it gives us more flexibility. Of course there is a question about the size of this buffer too. I am not sure about that because I have not checked yet how many log entries are created by the SINIT ACM. Though probably it should not be large... > memory. I am just wondering if, from security perspective, it will be > better to use memory from TXT heap for event log, like we do in TBOOT. Appendix F, TPM Event Log, has following sentence: There are no requirements for event log to be in DMA protected memory – SINIT will not enforce it. I was thinking about it and it seems to me that the TPM event log does not require any special protections. Any changes in it can be quickly detected by comparing hashes with the TPM PCRs. Does not it? >>> >>> I think it would be beneficial to consider the following in deciding >>> where the log is placed. There are two areas of attack/manipulation that >>> need to be considered. The first area is the log contents itself, which >>> as Daniel has pointed out, the log contents do not really need to be >>> protected from tampering as that would/should be detected during >>> verification by the attestor. The second area that we need to consider >>> is the log descriptors themselves. If these are in an area that can be >>> manipulated, it is an opportunity for an attacker to attempt to >>> influence the ACM's execution. For a little perspective, the ACM >>> executes from CRAM to take the most possible precaution to ensure that >>> it cannot be tampered with during execution. This is very important >>> since it runs a CPU mode (ACM Mode) that I would consider to be higher >>> (or lower depending on how you view it) than SMM. As a result, the txt >>> heap is also included in what is mapped into CRAM. If the event log is >>> place in the heap, this ensures that the ACM is not using memory outside >>> of CRAM during execution. Now as Daniel has pointed out, the down side >>> to this is that it greatly restricts the log size and can only be >>> managed by a combination of limiting the number of events and >>> restricting what content is carried in the event data field. >> >> Can you clarify what the actual flow of control is? If I had to guess, it's: >> >> GRUB (or other bootloader) writes log. >> >> GRUB transfers control to the ACM. At this point, GRUB is done >> running and GRUB code will not run again. >> >> ACM validates system configuration and updates TPM state using magic >> privileged TPM access. >> >> ACM transfers control to the shiny new Linux secure launch entry point >> >> Maybe this is right, and maybe this is wrong. But I have some >> questions about this whole setup. Is the ACM code going to inspect >> this log at all? If so, why? Who supplies the ACM code? If the ACM >> can be attacked by putting its inputs (e.g. this log) in the wrong >> place in memory, why should this be considered anything other than a >> bug in the ACM? > > There is a lot behind that, so to get a complete detail of the event > sequence I would recommend looking at Section Vol. 2D 6.2.3 (pg Vol. 2D > 6-5/ pdf pg 2531), 6.3 GETSEC[ENTERACCS](pg 6-10 Vol. 2D/pdf pg 2546), > and 6.3 GETSEC[SENTER](pg Vol. 2D 6-21/pdf pg 2557) in the Intel SDM[1]. > Section 6.2.3 gives a slightly detailed overview. Section > GETSEC[ENTERACCS] details the requirements/procedures for entering AC > execution mode and ACRAM (Authenticated CRAM) and section GETSEC[SENTER] > will detail requirements/procedures for SENTER. > > To answer you additional questions I would say if you look at all the > work that goes into protecting the ACM execution, why would you want to > then turn around and have it use memory outside of the protected region. > On the other hand, you are right, if the Developer's Guide says it > doesn't need to be protected and someone somehow finds a way to cause a > failure in the ACM through the use of a log outside of CRAM, then > rightfully that is a bug in the ACM. This is why I asked about making it > configurable, paranoid people could set the configuration to use the > heap and all others could just use an external location. After thinking about it, it should be easy to make it configurable since as stated it is up the the pre-launch code to decide where the buffer is. To do this
Re: [GRUB PATCH RFC 15/18] i386/txt: Add Intel TXT core implementation
On 5/22/20 9:24 AM, Krystian Hebel wrote:
>
> On 05.05.2020 01:21, Daniel Kiper wrote:
>> +static grub_err_t
>> +init_txt_heap (struct grub_slaunch_params *slparams, struct
>> grub_txt_acm_header *sinit)
>> +{
>> + grub_uint8_t *txt_heap;
>> + grub_uint32_t os_sinit_data_ver, sinit_caps;
>> + grub_uint64_t *size;
>> + struct grub_txt_os_mle_data *os_mle_data;
>> + struct grub_txt_os_sinit_data *os_sinit_data;
>> + struct grub_txt_heap_end_element *heap_end_element;
>> + struct grub_txt_heap_event_log_pointer2_1_element
>> *heap_event_log_pointer2_1_element;
>> +#ifdef GRUB_MACHINE_EFI
>> + struct grub_acpi_rsdp_v20 *rsdp;
>> +#endif
>> +
>> + /* BIOS data already verified in grub_txt_verify_platform(). */
>> +
>> + txt_heap = grub_txt_get_heap ();
>> +
>> + /* OS/loader to MLE data. */
>> +
>> + os_mle_data = grub_txt_os_mle_data_start (txt_heap);
>> + size = (grub_uint64_t *) ((grub_addr_t) os_mle_data - sizeof
>> (grub_uint64_t));
> There is 'grub_txt_os_mle_data_size()' in previous patch, it would look
> better.
>> + *size = sizeof (*os_mle_data) + sizeof (grub_uint64_t);
>> +
>> + grub_memset (os_mle_data, 0, sizeof (*os_mle_data));
>> +
>> + os_mle_data->version = GRUB_SL_OS_MLE_STRUCT_VERSION;
>> + os_mle_data->zero_page_addr = (grub_uint32_t)(grub_addr_t)
>> slparams->params;
>> + os_mle_data->saved_misc_enable_msr = grub_rdmsr
>> (GRUB_MSR_X86_MISC_ENABLE);
>> +
>> + os_mle_data->ap_wake_block = slparams->ap_wake_block;
>> +
>> + save_mtrrs (os_mle_data);
>> +
>> + /* OS/loader to SINIT data. */
>> +
>> + os_sinit_data_ver = grub_txt_supported_os_sinit_data_ver (sinit);
>> +
>> + if (os_sinit_data_ver < OS_SINIT_DATA_MIN_VER)
>> + return grub_error (GRUB_ERR_BAD_DEVICE,
>> + N_("unsupported OS to SINIT data version in SINIT ACM:
>> %d"
>> + " expected >= %d"), os_sinit_data_ver,
>> OS_SINIT_DATA_MIN_VER);
>> +
>> + os_sinit_data = grub_txt_os_sinit_data_start (txt_heap);
>> + size = (grub_uint64_t *) ((grub_addr_t) os_sinit_data - sizeof
>> (grub_uint64_t));
> Ditto
>> +
>> + *size = sizeof(grub_uint64_t) + sizeof (struct
>> grub_txt_os_sinit_data) +
>> + sizeof (struct grub_txt_heap_end_element);
>> +
>> + if (grub_get_tpm_ver () == GRUB_TPM_12)
>> + *size += sizeof (struct grub_txt_heap_tpm_event_log_element);
>> + else if (grub_get_tpm_ver () == GRUB_TPM_20)
>> + *size += sizeof (struct grub_txt_heap_event_log_pointer2_1_element);
>> + else
>> + return grub_error (GRUB_ERR_BAD_DEVICE, N_("unsupported TPM
>> version"));
>> +
>> + grub_memset (os_sinit_data, 0, *size);
>> +
>> +#ifdef GRUB_MACHINE_EFI
>> + rsdp = grub_acpi_get_rsdpv2 ();
>> +
>> + if (rsdp == NULL)
>> + return grub_printf ("WARNING: ACPI RSDP 2.0 missing\n");
>> +
>> + os_sinit_data->efi_rsdt_ptr = (grub_uint64_t)(grub_addr_t) rsdp;
>> +#endif
>> +
>> + os_sinit_data->mle_ptab = slparams->mle_ptab_target;
>> + os_sinit_data->mle_size = slparams->mle_size;
>> +
>> + os_sinit_data->mle_hdr_base = slparams->mle_header_offset;
>> +
>> + /* TODO: Check low PMR with RMRR. Look at relevant tboot code too. */
>> + /* TODO: Kernel should not allocate any memory outside of PMRs
>> regions!!! */
>> + os_sinit_data->vtd_pmr_lo_base = 0;
>> + os_sinit_data->vtd_pmr_lo_size = ALIGN_DOWN (grub_mmap_get_highest
>> (0x1),
>> + GRUB_TXT_PMR_ALIGN);
>> +
>> + os_sinit_data->vtd_pmr_hi_base = ALIGN_UP (grub_mmap_get_lowest
>> (0x1),
>> + GRUB_TXT_PMR_ALIGN);
>> + os_sinit_data->vtd_pmr_hi_size = ALIGN_DOWN (grub_mmap_get_highest
>> (0x),
>> + GRUB_TXT_PMR_ALIGN);
>> + os_sinit_data->vtd_pmr_hi_size -= os_sinit_data->vtd_pmr_hi_base;
> Could it be done with just one PMR, from 0 to top of memory, or would
No because there could be these legacy reserved regions in somewhere
below 4G that might cause hangs if you block DMA to them. So the idea is
the high PMR covers everything > 4G and the low PMR needs special logic
to not block these reserved regions. We are working on better logic for
this now.
> TXT complain?
>> +
>> + grub_dprintf ("slaunch",
>> + "vtd_pmr_lo_base: 0x%" PRIxGRUB_UINT64_T " vtd_pmr_lo_size: 0x%"
>> + PRIxGRUB_UINT64_T " vtd_pmr_hi_base: 0x%" PRIxGRUB_UINT64_T
>> + " vtd_pmr_hi_size: 0x%" PRIxGRUB_UINT64_T "\n",
>> + os_sinit_data->vtd_pmr_lo_base, os_sinit_data->vtd_pmr_lo_size,
>> + os_sinit_data->vtd_pmr_hi_base, os_sinit_data->vtd_pmr_hi_size);
>> +
>>
>>
>>
>> +/*
>> + * The MLE page tables have to be below the MLE and have no special
>> regions in
>> + * between them and the MLE (this is a bit of an unwritten rule).
>> + * 20 pages are carved out of memory below the MLE. That leave 18
>> page table
>> + * pages that can cover up to 36M .
>> + * can only contain 4k pages
>> + *
>> + * TODO: TXT Spec p.32; List section name and number with PT MLE
>> requirments here.
>> + *
>> + * TODO: This func
Re: [GRUB PATCH 1/1] loader/i386/linux: Fix an underflow in the setup_header length calculation
On 12/02/2019 12:29 PM, Daniel Kiper wrote:
> Recent work around x86 Linux kernel loader revealed an underflow in the
> setup_header length calculation and another related issue. Both lead to
> the memory overwrite and later machine crash.
>
> Currently when the GRUB copies the setup_header into the linux_params
> (struct boot_params, traditionally known as "zero page") it assumes the
> setup_header size as sizeof(linux_i386_kernel_header/lh). This is
> incorrect. It should use the value calculated accordingly to the Linux
> kernel boot protocol. Otherwise in case of pretty old kernel, to be
> exact Linux kernel boot protocol, the GRUB may write more into
> linux_params than it was expected to. Fortunately this is not very big
> issue. Though it has to be fixed. However, there is also an underflow
> which is grave. It happens when
>
> sizeof(linux_i386_kernel_header/lh) > "real size of the setup_header".
>
> Then len value wraps around and grub_file_read() reads whole kernel into
> the linux_params overwriting memory past it. This leads to the GRUB
> memory allocator breakage and finally to its crash during boot.
>
> The patch fixes both issues. Additionally, it moves the code not related to
> grub_memset(linux_params)/grub_memcpy(linux_params)/grub_file_read(linux_params)
> section outside of it to not confuse the reader.
>
> Signed-off-by: Daniel Kiper
Yes this looks correct. The length should be based off the jmp offset
byte and not the size of the structure defined in GRUB.
Reviewed-by: Ross Philipson
> ---
> grub-core/loader/i386/linux.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index d0501e229..ee95cd374 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -761,17 +761,12 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
> goto fail;
>
>grub_memset (&linux_params, 0, sizeof (linux_params));
> - grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, sizeof (lh) -
> 0x1F1);
> -
> - linux_params.code32_start = prot_mode_target + lh.code32_start -
> GRUB_LINUX_BZIMAGE_ADDR;
> - linux_params.kernel_alignment = (1 << align);
> - linux_params.ps_mouse = linux_params.padding10 = 0;
>
>/*
> * The Linux 32-bit boot protocol defines the setup header end
> * to be at 0x202 + the byte value at 0x201.
> */
> - len = 0x202 + *((char *) &linux_params.jump + 1);
> + len = 0x202 + *((char *) &lh.jump + 1);
>
>/* Verify the struct is big enough so we do not write past the end. */
>if (len > (char *) &linux_params.edd_mbr_sig_buffer - (char *)
> &linux_params) {
> @@ -779,10 +774,13 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
> goto fail;
>}
>
> + grub_memcpy (&linux_params.setup_sects, &lh.setup_sects, len - 0x1F1);
> +
>/* We've already read lh so there is no need to read it second time. */
>len -= sizeof(lh);
>
> - if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) !=
> len)
> + if ((len > 0) &&
> + (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) !=
> len))
> {
>if (!grub_errno)
> grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
> @@ -790,6 +788,9 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>goto fail;
> }
>
> + linux_params.code32_start = prot_mode_target + lh.code32_start -
> GRUB_LINUX_BZIMAGE_ADDR;
> + linux_params.kernel_alignment = (1 << align);
> + linux_params.ps_mouse = linux_params.padding10 = 0;
>linux_params.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
>
>/* These two are used (instead of cmd_line_ptr) by older versions of Linux,
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] loader/i386/linux: Calculate the setup_header length
On 04/01/2019 07:10 AM, Daniel Kiper wrote:
> On Fri, Mar 29, 2019 at 11:55:12AM -0400, Ross Philipson wrote:
>> On 03/29/2019 11:09 AM, Daniel Kiper wrote:
>>> From: Andrew Jeddeloh
>>>
>>> Previously the setup_header length was just assumed to be the size of the
>>> linux_kernel_params struct. The linux x86 32-bit boot protocol says that
>>> the size of the linux_i386_kernel_header is 0x202 + the byte value at 0x201
>>> in the linux_i386_kernel_header. Calculate the size of the header, rather
>>> than assume it is the size of the linux_kernel_params struct.
>>>
>>> Additionally, add some required members to the linux_kernel_params
>>> struct and align the content of linux_i386_kernel_header struct with
>>> it. New members naming was taken directly from Linux kernel source.
>>>
>>> linux_kernel_params and linux_i386_kernel_header structs require more
>>> cleanup. However, this is not urgent, so, let's do this after release.
>>> Just in case...
>>>
>>> Signed-off-by: Andrew Jeddeloh
>>> Signed-off-by: Daniel Kiper
>>> ---
>>> grub-core/loader/i386/linux.c | 16 +++-
>>> include/grub/i386/linux.h | 14 --
>>> 2 files changed, 27 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
>>> index b6015913b..73e15a90f 100644
>>> --- a/grub-core/loader/i386/linux.c
>>> +++ b/grub-core/loader/i386/linux.c
>>> @@ -767,7 +767,21 @@ grub_cmd_linux (grub_command_t cmd __attribute__
>>> ((unused)),
>>>linux_params.kernel_alignment = (1 << align);
>>>linux_params.ps_mouse = linux_params.padding10 = 0;
>>>
>>> - len = sizeof (linux_params) - sizeof (lh);
>>> + /*
>>> + * The Linux 32-bit boot protocol defines the setup header size
>>> + * to be 0x202 + the byte value at 0x201.
>>> + */
>>> + len = 0x202 + *((char *) &linux_params.jump + 1);
>>> +
>>> + /* Verify the struct is big enough so we do not write past the end. */
>>> + if (len > (char *) &linux_params.edd_mbr_sig_buffer - (char *)
>>> &linux_params) {
>>> +grub_error (GRUB_ERR_BAD_OS, "Linux setup header too big");
>>> +goto fail;
>>> + }
>>> +
>>> + /* We've already read lh so there is no need to read it second time. */
>>> + len -= sizeof(lh);
>>> +
>>
>> I am a little confused about the logic here and what exactly you are
>> trying to get the size of. The size of just the setup_header portion
>> would start at 0x1f1 so the logic for finding that would be something like:
>>
>> len = (0x202 - 0x1f1) + *((char *) &linux_params.jump + 1);
>>
>> If you are trying to find the size of all the boot params, your
>> calculation would leave out edd_mbr_sig_buffer and e820_map which are
>> after the end of the setup_header.
>
> Documentation/x86/boot.txt, 32-bit BOOT PROTOCOL section says:
>
> In 32-bit boot protocol, the first step in loading a Linux kernel
> should be to setup the boot parameters (struct boot_params,
> traditionally known as "zero page"). The memory for struct boot_params
> should be allocated and initialized to all zero. Then the setup header
> from offset 0x01f1 of kernel image on should be loaded into struct
> boot_params and examined. The end of setup header can be calculated as
> follow:
>
> 0x0202 + byte value at offset 0x0201
>
> The problem with currently existing GRUB code is that it reads data into
> linux_params past the end of lh. So, we have to properly calculate the
> end of lh using algorithm described above. Maybe the comment "The Linux
> 32-bit boot protocol defines the setup header size to be 0x202 + the
> byte value at 0x201." is a bit confusing and I should do s/size/end/
Yes calling it end would make it much clearer and that is what the
snippet of docs you pasted above calls it. Now I understand what you are
calculating.
>
>> Note I am using setup_header as a reference to the struct as defined in
>> linux. GRUB does not separate it out as it's own struct but just puts
>> the fields in the overall linux_kernel_params struct. Maybe it should be
>> broken out as a separate structure for clarity.
>
> In general both Linux boot structs are defined in the GRUB in very
> confusing way. So, I would suggest to be more tightly tied to the GRUB
> code and definitions than relevant code and definitions in the kernel.
>
Re: [PATCH] loader/i386/linux: Calculate the setup_header length
On 03/29/2019 11:09 AM, Daniel Kiper wrote:
> From: Andrew Jeddeloh
>
> Previously the setup_header length was just assumed to be the size of the
> linux_kernel_params struct. The linux x86 32-bit boot protocol says that
> the size of the linux_i386_kernel_header is 0x202 + the byte value at 0x201
> in the linux_i386_kernel_header. Calculate the size of the header, rather
> than assume it is the size of the linux_kernel_params struct.
>
> Additionally, add some required members to the linux_kernel_params
> struct and align the content of linux_i386_kernel_header struct with
> it. New members naming was taken directly from Linux kernel source.
>
> linux_kernel_params and linux_i386_kernel_header structs require more
> cleanup. However, this is not urgent, so, let's do this after release.
> Just in case...
>
> Signed-off-by: Andrew Jeddeloh
> Signed-off-by: Daniel Kiper
> ---
> grub-core/loader/i386/linux.c | 16 +++-
> include/grub/i386/linux.h | 14 --
> 2 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
> index b6015913b..73e15a90f 100644
> --- a/grub-core/loader/i386/linux.c
> +++ b/grub-core/loader/i386/linux.c
> @@ -767,7 +767,21 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> ((unused)),
>linux_params.kernel_alignment = (1 << align);
>linux_params.ps_mouse = linux_params.padding10 = 0;
>
> - len = sizeof (linux_params) - sizeof (lh);
> + /*
> + * The Linux 32-bit boot protocol defines the setup header size
> + * to be 0x202 + the byte value at 0x201.
> + */
> + len = 0x202 + *((char *) &linux_params.jump + 1);
> +
> + /* Verify the struct is big enough so we do not write past the end. */
> + if (len > (char *) &linux_params.edd_mbr_sig_buffer - (char *)
> &linux_params) {
> +grub_error (GRUB_ERR_BAD_OS, "Linux setup header too big");
> +goto fail;
> + }
> +
> + /* We've already read lh so there is no need to read it second time. */
> + len -= sizeof(lh);
> +
I am a little confused about the logic here and what exactly you are
trying to get the size of. The size of just the setup_header portion
would start at 0x1f1 so the logic for finding that would be something like:
len = (0x202 - 0x1f1) + *((char *) &linux_params.jump + 1);
If you are trying to find the size of all the boot params, your
calculation would leave out edd_mbr_sig_buffer and e820_map which are
after the end of the setup_header.
Note I am using setup_header as a reference to the struct as defined in
linux. GRUB does not separate it out as it's own struct but just puts
the fields in the overall linux_kernel_params struct. Maybe it should be
broken out as a separate structure for clarity.
>if (grub_file_read (file, (char *) &linux_params + sizeof (lh), len) !=
> len)
> {
>if (!grub_errno)
> diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
> index a96059311..ce30e7fb0 100644
> --- a/include/grub/i386/linux.h
> +++ b/include/grub/i386/linux.h
> @@ -46,6 +46,9 @@
> #define VIDEO_CAPABILITY_SKIP_QUIRKS (1 << 0)
> #define VIDEO_CAPABILITY_64BIT_BASE (1 << 1)/* Frame buffer base is
> 64-bit. */
>
> +/* Maximum number of MBR signatures to store. */
> +#define EDD_MBR_SIG_MAX 16
> +
> #ifdef __x86_64__
>
> #define GRUB_LINUX_EFI_SIGNATURE \
> @@ -142,6 +145,7 @@ struct linux_i386_kernel_header
>grub_uint64_t setup_data;
>grub_uint64_t pref_address;
>grub_uint32_t init_size;
> + grub_uint32_t handover_offset;
> } GRUB_PACKED;
>
> /* Boot parameters for Linux based on 2.6.12. This is used by the setup
> @@ -273,6 +277,7 @@ struct linux_kernel_params
>
>grub_uint8_t padding9[0x1f1 - 0x1e9];
>
> + /* Linux setup header copy - BEGIN. */
>grub_uint8_t setup_sects; /* The size of the setup in sectors */
>grub_uint16_t root_flags; /* If the root is mounted readonly */
>grub_uint16_t syssize; /* obsolete */
> @@ -311,9 +316,14 @@ struct linux_kernel_params
>grub_uint32_t payload_offset;
>grub_uint32_t payload_length;
>grub_uint64_t setup_data;
> - grub_uint8_t pad2[120];/* 258 */
> + grub_uint64_t pref_address;
> + grub_uint32_t init_size;
> + grub_uint32_t handover_offset;
Yea I noticed these were missing, good to add them. You should move that
comment down nd update it to /* 268 */
Also since you added these here, do you want to add them to
linux_i386_kernel_header for consistency? That struct is still stuck at
boot protocol verion 2.10 as stated in the comment.
> + /* Linux setup header copy - END. */
> +
> + grub_uint8_t _pad7[40];
> + grub_uint32_t edd_mbr_sig_buffer[EDD_MBR_SIG_MAX]; /* 290 */
>struct grub_e820_mmap e820_map[(0x400 - 0x2d0) / 20]; /* 2d0 */
> -
> } GRUB_PACKED;
> #endif /* ! ASM_FILE */
>
>
___
Grub-devel mailing list
Grub-devel@gnu.org
ht
Re: [PATCH 0/5] Various GRUB build fixes
On 03/19/2019 08:39 AM, Daniel Kiper wrote: > Hey, > > As in subject... I am CC-ing folks who were somehow involved in > doing/reviewing patches touching broken files. If I missed somebody > then sorry about that. Anyway, please take a look. > > There are still broken builds which I am going to fix soon. However, > fortunately most of our targets build without any issue right now. > > Daniel > > grub-core/kern/compiler-rt.c | 5 +++-- > grub-core/loader/ia64/efi/linux.c | 3 +-- > grub-core/loader/mips/linux.c | 2 +- > grub-core/loader/powerpc/ieee1275/linux.c | 4 ++-- > include/grub/compiler-rt.h| 4 +++- > 5 files changed, 10 insertions(+), 8 deletions(-) > > Daniel Kiper (5): > verifiers: IA-64 fallout cleanup It seems like the things fixed in these two would have prevented compiling. I guess it was not tried on these platforms? > verifiers: PowerPC fallout cleanup > verifiers: MIPS fallout cleanup> mips: Enable __clzsi2() > sparc: Enable __clzsi2() and __clzdi2() > Anyway, LGTM Reviewed-by: Ross Philipson ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [GRUB PATCH 1/2] verifiers: Xen fallout cleanup
On 12/06/2018 10:40 AM, Daniel Kiper wrote: > On Thu, Dec 06, 2018 at 10:37:43AM -0500, Ross Philipson wrote: >> On 12/06/2018 08:40 AM, Daniel Kiper wrote: >>> Xen fallout cleanup after commit ca0a4f689 (verifiers: File type for >>> fine-grained signature-verification controlling). >>> >>> Signed-off-by: Daniel Kiper >>> --- >>> grub-core/loader/i386/xen.c | 14 +++--- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c >>> index 1a99ca72c..8f662c8ac 100644 >>> --- a/grub-core/loader/i386/xen.c >>> +++ b/grub-core/loader/i386/xen.c >>> @@ -645,10 +645,10 @@ grub_cmd_xen (grub_command_t cmd __attribute__ >>> ((unused)), >>> >>>grub_xen_reset (); >>> >>> - grub_create_loader_cmdline (argc - 1, argv + 1, >>> - (char *) xen_state.next_start.cmd_line, >>> - sizeof (xen_state.next_start.cmd_line) - 1); >>> - err = grub_verify_string (xen_state.next_start.cmd_line, >>> GRUB_VERIFY_MODULE_CMDLINE); >>> + err = grub_create_loader_cmdline (argc - 1, argv + 1, >>> + (char *) xen_state.next_start.cmd_line, >>> + sizeof (xen_state.next_start.cmd_line) - 1, >>> + GRUB_VERIFY_KERNEL_CMDLINE); >> >> How did this compile previously if you were missing an argument to >> grub_create_loader_cmdline? > > This is only build if xen platform is enabled. Otherwise this file is > not used. Ack, that is what I was starting to guess happened. For the series: Reviewed-by: Ross Philipson > > Daniel > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [GRUB PATCH 1/2] verifiers: Xen fallout cleanup
On 12/06/2018 08:40 AM, Daniel Kiper wrote: > Xen fallout cleanup after commit ca0a4f689 (verifiers: File type for > fine-grained signature-verification controlling). > > Signed-off-by: Daniel Kiper > --- > grub-core/loader/i386/xen.c | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c > index 1a99ca72c..8f662c8ac 100644 > --- a/grub-core/loader/i386/xen.c > +++ b/grub-core/loader/i386/xen.c > @@ -645,10 +645,10 @@ grub_cmd_xen (grub_command_t cmd __attribute__ > ((unused)), > >grub_xen_reset (); > > - grub_create_loader_cmdline (argc - 1, argv + 1, > - (char *) xen_state.next_start.cmd_line, > - sizeof (xen_state.next_start.cmd_line) - 1); > - err = grub_verify_string (xen_state.next_start.cmd_line, > GRUB_VERIFY_MODULE_CMDLINE); > + err = grub_create_loader_cmdline (argc - 1, argv + 1, > + (char *) xen_state.next_start.cmd_line, > + sizeof (xen_state.next_start.cmd_line) - 1, > + GRUB_VERIFY_KERNEL_CMDLINE); How did this compile previously if you were missing an argument to grub_create_loader_cmdline? >if (err) > return err; > > @@ -910,9 +910,9 @@ grub_cmd_module (grub_command_t cmd __attribute__ > ((unused)), >if (err) > goto fail; > > - grub_create_loader_cmdline (argc - 1, argv + 1, > - get_virtual_current_address (ch), cmdline_len); > - err = grub_verify_string (get_virtual_current_address (ch), > GRUB_VERIFY_MODULE_CMDLINE); > + err = grub_create_loader_cmdline (argc - 1, argv + 1, > + get_virtual_current_address (ch), > cmdline_len, > + GRUB_VERIFY_MODULE_CMDLINE); >if (err) > goto fail; > > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [GRUB PATCH 2/2] verifiers: ARM Xen fallout cleanup
On 12/06/2018 08:40 AM, Daniel Kiper wrote:
> ARM Xen fallout cleanup after commit ca0a4f689 (verifiers: File type for
> fine-grained signature-verification controlling).
>
> Signed-off-by: Daniel Kiper
> ---
> grub-core/loader/arm64/xen_boot.c | 8
> include/grub/file.h | 5 +
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/loader/arm64/xen_boot.c
> b/grub-core/loader/arm64/xen_boot.c
> index 33a855df4..a742868a4 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -429,9 +429,9 @@ grub_cmd_xen_module (grub_command_t cmd
> __attribute__((unused)),
>
>grub_dprintf ("xen_loader", "Init module and node info\n");
>
> - if (nounzip)
> -grub_file_filter_disable_compression ();
> - file = grub_file_open (argv[0]);
> + file = grub_file_open (argv[0], GRUB_FILE_TYPE_XEN_MODULE
> + | (nounzip ? GRUB_FILE_TYPE_NO_DECOMPRESS
> + : GRUB_FILE_TYPE_NONE));
Same question, how did this compile if you were missing an argument? I
guess maybe you were not building xen bits in and you missed fixing this up?
>if (!file)
> goto fail;
>
> @@ -463,7 +463,7 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__
> ((unused)),
>goto fail;
> }
>
> - file = grub_file_open (argv[0]);
> + file = grub_file_open (argv[0], GRUB_FILE_TYPE_XEN_HYPERVISOR);
>if (!file)
> goto fail;
>
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 9aae46355..cbbd29465 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -42,6 +42,11 @@ enum grub_file_type
> /* Multiboot module. */
> GRUB_FILE_TYPE_MULTIBOOT_MODULE,
>
> +/* Xen hypervisor - used on ARM only. */
> +GRUB_FILE_TYPE_XEN_HYPERVISOR,
> +/* Xen module - used on ARM only. */
> +GRUB_FILE_TYPE_XEN_MODULE,
> +
> GRUB_FILE_TYPE_BSD_KERNEL,
> GRUB_FILE_TYPE_FREEBSD_ENV,
> GRUB_FILE_TYPE_FREEBSD_MODULE,
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 0/9] verifiers: Framework and EFI shim lock verifier
+- > grub-core/video/readers/jpeg.c |2 +- > grub-core/video/readers/png.c|2 +- > grub-core/video/readers/tga.c|2 +- > include/grub/bufio.h |6 +- > include/grub/dl.h| 13 + > include/grub/elfload.h |2 +- > include/grub/file.h | 154 ++-- > include/grub/lib/cmdline.h |5 +- > include/grub/list.h |1 + > include/grub/machoload.h |3 +- > include/grub/verify.h| 78 ++ > util/grub-fstest.c |6 +- > util/grub-mount.c|6 +- > 88 files changed, 1949 insertions(+), 1282 deletions(-) > > Daniel Kiper (5): > bufio: Use grub_size_t instead of plain int for size > verifiers: Add possibility to defer verification to other verifiers > verifiers: Rename verify module to pgp module > dl: Add support for persistent modules > efi: Add EFI shim lock verifier > > Vladimir Serbinenko (4): > verifiers: File type for fine-grained signature-verification controlling > verifiers: Framework core > verifiers: Add possibility to verify kernel and modules command lines > verifiers: Add the documentation > This version of the patch set looks good to me. Reviewed-by: Ross Philipson ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 6/8] verifiers: Add the documentation
On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> From: Vladimir Serbinenko
>
> Signed-off-by: Vladimir Serbinenko
> Signed-off-by: Daniel Kiper
> ---
> v3 - suggestions/fixes:
>- improve the documentation.
> ---
> docs/grub-dev.texi | 57
>
> 1 file changed, 57 insertions(+)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index a9f4de6..ad72705 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -84,6 +84,7 @@ This edition documents version @value{VERSION}.
> * Video Subsystem::
> * PFF2 Font File Format::
> * Graphical Menu Software Design::
> +* Verifiers framework::
> * Copying This Manual:: Copying This Manual
> * Index::
> @end menu
> @@ -1949,6 +1950,62 @@ the graphics mode that was in use before
> @code{grub_video_setup()} was called
> might fix some of the problems.
>
>
> +@node Verifiers framework
> +@chapter Verifiers framework
> +
> +To register your own verifier call @samp{grub_verifier_register} with a
> +structure pointing to your functions.
> +
> +The interface is inspired by hash interface with
> @samp{init}/@samp{write}/@samp{fini}.
"... by the hash ..."
> +> +There are eesntially 2 ways of using it: hashing and whole-file
verification:
eesntially -> essentially
The first : could be replaced with a , or ; depending on your mood :)
> +
> +With hashing approach:
"With the hashing approach:"
> +During @samp{init} you decide whether you want to check given file and init
> context.
"... check the given ..."
> +In @samp{write} you update you hashing state.
"you hashing" -> "your hasking"
> +In @samp{fini} you check that hash matches the expected value/passes some
> check/...
"... that the hash ..."
> +
> +With whole-file verification:
> +During @samp{init} you decide whether you want to check given file and init
> context.
"... check the given ..."
> +In @samp{write} you verify file and return error if it fails.
"... verify the file and return an error ..."
> +You don't have @samp{fini}.
> +
> +Additional @samp{verify_string} receives various strings like kernel
> parameters to
> +verify. Returning no error means successful verification and an error stops
> the current
> +action.
> +
> +Detailed description of API:
"... of the API ..."
> +
> +Every time a file is opened your @samp{init} function is called with file
> descriptor
"... a file descriptor..."
> +and file type. Your function can have following outcomes:
"... have the following ..."
> +
> +@itemize
> +
> +@item returning no error and setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_DEFER}.
> +In this case verification is deferred to others active verifiers.
> Verification fails if
others -> other
> +nobody cares or selected verifier fails
> +
> +@item returning no error and setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}.
> +In this case your verifier will not be called anymore and your verifier is
> considered
considered -> assumed
> +to have skipped verification
> +
> +@item returning error. Then opening of the file will fail due to failed
> verification.
> +
> +@item returning no error and not setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}
> +In this case verification is done as described in following section
"... in the following ..."
> +
> +@end itemize
> +
> +In the fourth case your @samp{write} will be called with chunks of file. If
> you need the whole file in a single
"... of the file ..."
> +chunk then during @samp{init} set bit @samp{GRUB_VERIFY_FLAGS_SINGLE_CHUNK}
> in @samp{*flags}.
"... set the bit..."
> +During @samp{init} you may set @samp{*context} if you need additional
> context. At every iteration you may return
> +an error and the the file will be considered as having failed the
> verification. If you return no error then
> +verification continues.
> +
> +Optionally at the end of the file @samp{fini} if it exists is called with
> just the context. If you return
You may need some commas around "if it exists" but I am not 100% sure
what is intended here.
> +no error during any of @samp{init}, @samp{write} and @samp{fini} then the
> file is considered as having
> +succeded verification.
> +
> @node Copying This Manual
> @appendix Copying This Manual
>
>
Just my suggestions. Since the original text went back and forth between
complete grammar and abbreviated grammar, it is probably best to make it
all consistent.
Thanks
Ross
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 6/8] verifiers: Add the documentation
On 10/09/2018 10:26 AM, Daniel Kiper wrote:
> On Fri, Oct 05, 2018 at 12:43:08PM -0400, Ross Philipson wrote:
>> On 10/03/2018 05:36 AM, Daniel Kiper wrote:
>>> From: Vladimir Serbinenko
>>>
>>> Signed-off-by: Vladimir Serbinenko
>>> Signed-off-by: Daniel Kiper
>>> ---
>>> v3 - suggestions/fixes:
>>>- improve the documentation.
>>> ---
>>> docs/grub-dev.texi | 57
>>>
>>> 1 file changed, 57 insertions(+)
>>>
>>> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
>>> index a9f4de6..ad72705 100644
>>> --- a/docs/grub-dev.texi
>>> +++ b/docs/grub-dev.texi
>>> @@ -84,6 +84,7 @@ This edition documents version @value{VERSION}.
>>> * Video Subsystem::
>>> * PFF2 Font File Format::
>>> * Graphical Menu Software Design::
>>> +* Verifiers framework::
>>> * Copying This Manual:: Copying This Manual
>>> * Index::
>>> @end menu
>>> @@ -1949,6 +1950,62 @@ the graphics mode that was in use before
>>> @code{grub_video_setup()} was called
>>> might fix some of the problems.
>>>
>>>
>>> +@node Verifiers framework
>>> +@chapter Verifiers framework
>>> +
>>> +To register your own verifier call @samp{grub_verifier_register} with a
>>> +structure pointing to your functions.
>>> +
>>> +The interface is inspired by hash interface with
>>> @samp{init}/@samp{write}/@samp{fini}.
>>> +
>>> +There are eesntially 2 ways of using it: hashing and whole-file
>>> verification:
>>
>> First : should be a ;
>>
>>> +
>>> +With hashing approach:
>>> +During @samp{init} you decide whether you want to check given file and
>>> init context.
>>> +In @samp{write} you update you hashing state.
>>
>> "update your..."
>>
>>> +In @samp{fini} you check that hash matches the expected value/passes some
>>> check/...
>>> +
>>> +With whole-file verification:
>>> +During @samp{init} you decide whether you want to check given file and
>>> init context.
>>> +In @samp{write} you verify file and return error if it fails.
>>> +You don't have @samp{fini}.
>>> +
>>> +Additional @samp{verify_string} receives various strings like kernel
>>> parameters to
>>> +verify. Returning no error means successful verification and an error
>>> stops the current
>>
>> s/and/or maybe?
>>
>>> +action.
>>> +
>>> +Detailed description of API:
>>> +
>>> +Every time a file is opened your @samp{init} function is called with file
>>> descriptor
>>> +and file type. Your function can have following outcomes:
>>> +
>>> +@itemize
>>> +
>>> +@item returning no error and setting @samp{*flags} to
>>> @samp{GRUB_VERIFY_FLAGS_DEFER}.
>>> +In this case verification is deferred to others active verifiers.
>>> Verification fails if
>>> +nobody cares or selected verifier fails
>>> +
>>> +@item returning no error and setting @samp{*flags} to
>>> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}.
>>> +In this case your verifier will not be called anymore and your verifier is
>>> considered
>>> +to have skipped verification
>>> +
>>> +@item returning error. Then opening of the file will fail due to failed
>>> verification.
>>> +
>>> +@item returning no error and not setting @samp{*flags} to
>>> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}
>>> +In this case verification is done as described in following section
>>> +
>>> +@end itemize
>>> +
>>> +In the fourth case your @samp{write} will be called with chunks of file.
>>> If you need the whole file in a single
>>> +chunk then during @samp{init} set bit
>>> @samp{GRUB_VERIFY_FLAGS_SINGLE_CHUNK} in @samp{*flags}.
>>> +During @samp{init} you may set @samp{*context} if you need additional
>>> context. At every iteration you may return
>>> +an error and the the file will be considered as having failed the
>>> verification. If you return no error then
>>> +verification continues.
>>> +
>>> +Optionally at the end of the file @samp{fini} if it exists is called with
>>> just the context. If you return
>>> +no error during any of @samp{init}, @samp{write} and @samp{fini} then the
>>>
Re: [PATCH v3 5/8] verifiers: Rename verify module to pgp module
On 10/09/2018 10:20 AM, Michel Hermier wrote: > That said maybe it's better for readability to do it in 2 patch, one for > moving the file and one for the renaming. I did a test and I thought format generated something more sane but maybe I only ended up doing "git show". Maybe just put a note under the snip line that format-patch is turning a "git mv" into a ton of duplicated patch code. Thanks ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 6/8] verifiers: Add the documentation
On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> From: Vladimir Serbinenko
>
> Signed-off-by: Vladimir Serbinenko
> Signed-off-by: Daniel Kiper
> ---
> v3 - suggestions/fixes:
>- improve the documentation.
> ---
> docs/grub-dev.texi | 57
>
> 1 file changed, 57 insertions(+)
>
> diff --git a/docs/grub-dev.texi b/docs/grub-dev.texi
> index a9f4de6..ad72705 100644
> --- a/docs/grub-dev.texi
> +++ b/docs/grub-dev.texi
> @@ -84,6 +84,7 @@ This edition documents version @value{VERSION}.
> * Video Subsystem::
> * PFF2 Font File Format::
> * Graphical Menu Software Design::
> +* Verifiers framework::
> * Copying This Manual:: Copying This Manual
> * Index::
> @end menu
> @@ -1949,6 +1950,62 @@ the graphics mode that was in use before
> @code{grub_video_setup()} was called
> might fix some of the problems.
>
>
> +@node Verifiers framework
> +@chapter Verifiers framework
> +
> +To register your own verifier call @samp{grub_verifier_register} with a
> +structure pointing to your functions.
> +
> +The interface is inspired by hash interface with
> @samp{init}/@samp{write}/@samp{fini}.
> +
> +There are eesntially 2 ways of using it: hashing and whole-file verification:
First : should be a ;
> +
> +With hashing approach:
> +During @samp{init} you decide whether you want to check given file and init
> context.
> +In @samp{write} you update you hashing state.
"update your..."
> +In @samp{fini} you check that hash matches the expected value/passes some
> check/...
> +
> +With whole-file verification:
> +During @samp{init} you decide whether you want to check given file and init
> context.
> +In @samp{write} you verify file and return error if it fails.
> +You don't have @samp{fini}.
> +
> +Additional @samp{verify_string} receives various strings like kernel
> parameters to
> +verify. Returning no error means successful verification and an error stops
> the current
s/and/or maybe?
> +action.
> +
> +Detailed description of API:
> +
> +Every time a file is opened your @samp{init} function is called with file
> descriptor
> +and file type. Your function can have following outcomes:
> +
> +@itemize
> +
> +@item returning no error and setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_DEFER}.
> +In this case verification is deferred to others active verifiers.
> Verification fails if
> +nobody cares or selected verifier fails
> +
> +@item returning no error and setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}.
> +In this case your verifier will not be called anymore and your verifier is
> considered
> +to have skipped verification
> +
> +@item returning error. Then opening of the file will fail due to failed
> verification.
> +
> +@item returning no error and not setting @samp{*flags} to
> @samp{GRUB_VERIFY_FLAGS_SKIP_VERIFICATION}
> +In this case verification is done as described in following section
> +
> +@end itemize
> +
> +In the fourth case your @samp{write} will be called with chunks of file. If
> you need the whole file in a single
> +chunk then during @samp{init} set bit @samp{GRUB_VERIFY_FLAGS_SINGLE_CHUNK}
> in @samp{*flags}.
> +During @samp{init} you may set @samp{*context} if you need additional
> context. At every iteration you may return
> +an error and the the file will be considered as having failed the
> verification. If you return no error then
> +verification continues.
> +
> +Optionally at the end of the file @samp{fini} if it exists is called with
> just the context. If you return
> +no error during any of @samp{init}, @samp{write} and @samp{fini} then the
> file is considered as having
> +succeded verification.
succeeded
> +
> @node Copying This Manual
> @appendix Copying This Manual
>
>
I noticed a lot of the text is missing articles, mainly "the" in a lot
of places. Not sure if this was intentional to keep the text more
abbreviated or not.
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 5/8] verifiers: Rename verify module to pgp module
On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> Just for clarity. No functional change.
>
> Signed-off-by: Daniel Kiper
> ---
> grub-core/Makefile.core.def |4 +-
> grub-core/commands/pgp.c| 1018
> +++
> grub-core/commands/verify.c | 1018
> ---
> 3 files changed, 1020 insertions(+), 1020 deletions(-)
> create mode 100644 grub-core/commands/pgp.c
> delete mode 100644 grub-core/commands/verify.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index dfcc95d..3008b58 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -888,8 +888,8 @@ module = {
> };
>
> module = {
> - name = verify;
> - common = commands/verify.c;
> + name = pgp;
> + common = commands/pgp.c;
>cflags = '$(CFLAGS_POSIX)';
>cppflags = '-I$(srcdir)/lib/posix_wrap';
> };
> diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
> new file mode 100644
> index 000..d5d7c0f
> --- /dev/null
> +++ b/grub-core/commands/pgp.c
> @@ -0,0 +1,1018 @@
[snip]
> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
> deleted file mode 100644
> index d5d7c0f..000
> --- a/grub-core/commands/verify.c
> +++ /dev/null
> @@ -1,1018 +0,0 @@
[snip]
Scanning through this in a split view, the new file looks exactly the
same as the old one. Shouldn't this just be a git mv (a rename commit)
instead of deleting the old file and adding a new one?
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 4/8] verifiers: Add possibility to defer verification to other verifiers
On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> This way if a verifier requires verification of a given file it can
> defer task to other verifier if it is not able to do it itself. E.g.
> shim_lock verifier, posted as a subsequent patch, is able to verify
> only PE files. This means that it is not able to verify any of GRUB2
> modules which have to be trusted on UEFI systems with secure boot
> enabled. So, it can defer verification to other verifier, e.g. PGP one.
>
> I silently assume that other verifiers are trusted and will do good
> job for us. Or at least they will not do any harm.
>
> Signed-off-by: Daniel Kiper
> ---
> grub-core/commands/verify_helper.c | 23 ---
> include/grub/verify.h |3 ++-
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/commands/verify_helper.c
> b/grub-core/commands/verify_helper.c
> index 7effc5f..ba8b03d 100644
> --- a/grub-core/commands/verify_helper.c
> +++ b/grub-core/commands/verify_helper.c
> @@ -83,6 +83,7 @@ grub_verify_helper_open (grub_file_t io, enum
> grub_file_type type)
>void *context;
>grub_file_t ret = 0;
>grub_err_t err;
> + int defer = 0;
>
>grub_dprintf ("verify", "file: %s type: %d\n", io->name, type);
>
> @@ -102,13 +103,27 @@ grub_verify_helper_open (grub_file_t io, enum
> grub_file_type type)
>err = ver->init (io, type, &context, &flags);
>if (err)
> goto fail_noclose;
> + if (flags & GRUB_VERIFY_FLAGS_DEFER)
> + {
> + defer = 1;
> + continue;
> + }
>if (!(flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION))
> break;
> }
>
>if (!ver)
> -/* No verifiers wanted to verify. Just return underlying file. */
> -return io;
> +{
> + if (defer)
> + {
> + grub_error (GRUB_ERR_ACCESS_DENIED,
> + N_("verification requested but nobody cares: %s"),
> io->name);
> + goto fail_noclose;
> + }
I don't really understand the logic here. Is the idea that if it was
deferred and no one in the chain verified it, it ends up being an error?
Why is that?
Also defer has different meanings. I think in this context it means
defer verification to another authority as opposed to defer until later.
> +
> + /* No verifiers wanted to verify. Just return underlying file. */
> + return io;
> +}
>
>ret = grub_malloc (sizeof (*ret));
>if (!ret)
> @@ -160,7 +175,9 @@ grub_verify_helper_open (grub_file_t io, enum
> grub_file_type type)
>err = ver->init (io, type, &context, &flags);
>if (err)
> goto fail_noclose;
> - if (flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION)
> + if (flags & GRUB_VERIFY_FLAGS_SKIP_VERIFICATION ||
> + /* Verification done earlier. So, we are happy here. */
> + flags & GRUB_VERIFY_FLAGS_DEFER)
> continue;
>err = ver->write (context, verified->buf, ret->size);
>if (err)
> diff --git a/include/grub/verify.h b/include/grub/verify.h
> index 9f892d8..c385e3d 100644
> --- a/include/grub/verify.h
> +++ b/include/grub/verify.h
> @@ -22,7 +22,8 @@
> enum grub_verify_flags
>{
> GRUB_VERIFY_FLAGS_SKIP_VERIFICATION = 1,
> -GRUB_VERIFY_FLAGS_SINGLE_CHUNK = 2
> +GRUB_VERIFY_FLAGS_SINGLE_CHUNK = 2,
> +GRUB_VERIFY_FLAGS_DEFER = 4
What is the difference between skip verification and defer?
>};
>
> enum grub_verify_string_type
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3 2/8] verifiers: Framework core
On 10/03/2018 05:36 AM, Daniel Kiper wrote:
> From: Vladimir Serbinenko
>
> Verifiers framework provides core file verification functionality which
> can be used by various security mechanisms, e.g., UEFI secure boot, TPM,
> PGP signature verification, etc.
>
> The patch contains PGP code changes and probably they should be extracted
> to separate patch for the sake of clarity.
>
> Signed-off-by: Vladimir Serbinenko
> Signed-off-by: Daniel Kiper
> ---
> v3 - suggestions/fixes:
>- minor improvement and cleanups.
> ---
> grub-core/Makefile.core.def|5 +
> grub-core/commands/verify.c| 565
> +---
> grub-core/commands/verify_helper.c | 197 +
> include/grub/file.h|2 +-
> include/grub/list.h|1 +
> include/grub/verify.h | 65 +
> 6 files changed, 538 insertions(+), 297 deletions(-)
> create mode 100644 grub-core/commands/verify_helper.c
> create mode 100644 include/grub/verify.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 9590e87..dfcc95d 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -895,6 +895,11 @@ module = {
> };
>
> module = {
> + name = verify_helper;
> + common = commands/verify_helper.c;
> +};
> +
> +module = {
>name = hdparm;
>common = commands/hdparm.c;
>enable = pci;
> diff --git a/grub-core/commands/verify.c b/grub-core/commands/verify.c
> index 3616318..d5d7c0f 100644
> --- a/grub-core/commands/verify.c
> +++ b/grub-core/commands/verify.c
> @@ -30,16 +30,10 @@
> #include
> #include
> #include
> +#include
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> -struct grub_verified
> -{
> - grub_file_t file;
> - void *buf;
> -};
> -typedef struct grub_verified *grub_verified_t;
> -
> enum
>{
> OPTION_SKIP_SIG = 0
> @@ -445,22 +439,26 @@ rsa_pad (gcry_mpi_t *hmpi, grub_uint8_t *hval,
>return ret;
> }
>
> -static grub_err_t
> -grub_verify_signature_real (char *buf, grub_size_t size,
> - grub_file_t f, grub_file_t sig,
> - struct grub_public_key *pkey)
> +struct grub_pubkey_context
> {
> - grub_size_t len;
> + grub_file_t sig;
> + struct signature_v4_header v4;
>grub_uint8_t v;
> + const gcry_md_spec_t *hash;
> + void *hash_context;
> +};
> +
> +static grub_err_t
> +grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t
> sig)
> +{
> + grub_size_t len;
>grub_uint8_t h;
>grub_uint8_t t;
> - grub_uint8_t pk;
> - const gcry_md_spec_t *hash;
> - struct signature_v4_header v4;
>grub_err_t err;
> - grub_size_t i;
> - gcry_mpi_t mpis[10];
>grub_uint8_t type = 0;
> + grub_uint8_t pk;
> +
> + grub_memset (ctxt, 0, sizeof (*ctxt));
>
>err = read_packet_header (sig, &type, &len);
>if (err)
> @@ -469,18 +467,18 @@ grub_verify_signature_real (char *buf, grub_size_t size,
>if (type != 0x2)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>
> - if (grub_file_read (sig, &v, sizeof (v)) != sizeof (v))
> + if (grub_file_read (sig, &ctxt->v, sizeof (ctxt->v)) != sizeof (ctxt->v))
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>
> - if (v != 4)
> + if (ctxt->v != 4)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>
> - if (grub_file_read (sig, &v4, sizeof (v4)) != sizeof (v4))
> + if (grub_file_read (sig, &ctxt->v4, sizeof (ctxt->v4)) != sizeof
> (ctxt->v4))
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>
> - h = v4.hash;
> - t = v4.type;
> - pk = v4.pkeyalgo;
> + h = ctxt->v4.hash;
> + t = ctxt->v4.type;
> + pk = ctxt->v4.pkeyalgo;
>
>if (t != 0)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
> @@ -491,183 +489,232 @@ grub_verify_signature_real (char *buf, grub_size_t
> size,
>if (pk >= ARRAY_SIZE (pkalgos) || pkalgos[pk].name == NULL)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>
> - hash = grub_crypto_lookup_md_by_name (hashes[h]);
> - if (!hash)
> + ctxt->hash = grub_crypto_lookup_md_by_name (hashes[h]);
> + if (!ctxt->hash)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, "hash `%s' not loaded",
> hashes[h]);
>
>grub_dprintf ("crypt", "alive\n");
>
> - {
> -void *context = NULL;
> -unsigned char *hval;
> -grub_ssize_t rem = grub_be_to_cpu16 (v4.hashed_sub);
> -grub_uint32_t headlen = grub_cpu_to_be32 (rem + 6);
> -grub_uint8_t s;
> -grub_uint16_t unhashed_sub;
> -grub_ssize_t r;
> -grub_uint8_t hash_start[2];
> -gcry_mpi_t hmpi;
> -grub_uint64_t keyid = 0;
> -struct grub_public_subkey *sk;
> -grub_uint8_t *readbuf = NULL;
> + ctxt->sig = sig;
>
> -context = grub_zalloc (hash->contextsize);
> -readbuf = grub_zalloc (READBUF_SIZE);
> -if (!context || !readbuf)
> - goto fail;
> -
> -hash->init
Re: [PATCH v3 1/8] verifiers: File type for fine-grained signature-verification controlling
On 10/03/2018 05:36 AM, Daniel Kiper wrote: > From: Vladimir Serbinenko > > Let's provide file type info to the I/O layer. This way verifiers > framework and its users will be able to differentiate files and verify > only required ones. > > This is preparatory patch. > > Signed-off-by: Vladimir Serbinenko > Signed-off-by: Daniel Kiper > --- > v3 - suggestions/fixes: >- remove unneeded grub-core/commands/i386/nthibr.c > (suggested by Matthew Garrett). > --- > grub-core/commands/acpi.c|2 +- > grub-core/commands/blocklist.c |4 +- > grub-core/commands/cat.c |2 +- > grub-core/commands/cmp.c |4 +- > grub-core/commands/efi/loadbios.c|4 +- > grub-core/commands/file.c|5 +- > grub-core/commands/hashsum.c | 22 ++-- > grub-core/commands/hexdump.c |2 +- > grub-core/commands/i386/pc/play.c|2 +- > grub-core/commands/keylayouts.c |2 +- > grub-core/commands/legacycfg.c |2 +- > grub-core/commands/loadenv.c | 24 ++-- > grub-core/commands/ls.c |8 +- > grub-core/commands/minicmd.c |2 +- > grub-core/commands/nativedisk.c |3 +- > grub-core/commands/parttool.c|2 +- > grub-core/commands/search.c |4 +- > grub-core/commands/test.c|4 +- > grub-core/commands/testload.c|2 +- > grub-core/commands/testspeed.c |2 +- > grub-core/commands/verify.c | 53 - > grub-core/disk/loopback.c|3 +- > grub-core/efiemu/main.c |2 +- > grub-core/font/font.c|4 +- > grub-core/fs/zfs/zfscrypt.c |2 +- > grub-core/gettext/gettext.c |2 +- > grub-core/gfxmenu/theme_loader.c |2 +- > grub-core/io/bufio.c | 10 +- > grub-core/io/gzio.c |5 +- > grub-core/io/lzopio.c|6 +- > grub-core/io/offset.c|7 +- > grub-core/io/xzio.c |6 +- > grub-core/kern/dl.c |2 +- > grub-core/kern/elf.c |4 +- > grub-core/kern/file.c| 22 ++-- > grub-core/lib/syslinux_parse.c |2 +- > grub-core/loader/efi/chainloader.c |2 +- > grub-core/loader/i386/bsd.c | 16 +-- > grub-core/loader/i386/coreboot/chainloader.c |2 +- > grub-core/loader/i386/linux.c|2 +- > grub-core/loader/i386/pc/chainloader.c |4 +- > grub-core/loader/i386/pc/freedos.c |2 +- > grub-core/loader/i386/pc/linux.c |2 +- > grub-core/loader/i386/pc/ntldr.c |2 +- > grub-core/loader/i386/pc/plan9.c |2 +- > grub-core/loader/i386/pc/pxechainloader.c|2 +- > grub-core/loader/i386/pc/truecrypt.c |2 +- > grub-core/loader/i386/xen.c |7 +- > grub-core/loader/i386/xen_file.c |2 +- > grub-core/loader/i386/xnu.c |2 +- > grub-core/loader/linux.c |6 +- > grub-core/loader/macho.c |4 +- > grub-core/loader/mips/linux.c|2 +- > grub-core/loader/multiboot.c |8 +- > grub-core/loader/xnu.c | 16 +-- > grub-core/loader/xnu_resume.c|4 +- > grub-core/normal/autofs.c| 11 +- > grub-core/normal/crypto.c|2 +- > grub-core/normal/dyncmd.c|2 +- > grub-core/normal/main.c |2 +- > grub-core/normal/term.c |2 +- > grub-core/video/readers/jpeg.c |2 +- > grub-core/video/readers/png.c|2 +- > grub-core/video/readers/tga.c|2 +- > include/grub/bufio.h |6 +- > include/grub/elfload.h |2 +- > include/grub/file.h | 151 > +++--- > include/grub/machoload.h |3 +- > util/grub-fstest.c |6 +- > util/grub-mount.c|6 +- > 70 files changed, 297 insertions(+), 225 deletions(-) > > diff --git a/grub-core/commands/acpi.c b/grub-core/commands/acpi.c > index 9f02f22..5a1499a 100644 > --- a/grub-core/commands/acpi.c > +++ b/grub-core/commands/acpi.c > @@ -635,7 +635,7 @@ grub_cmd_acpi (struct grub_extcmd_context *ctxt, int > argc, char **args) >grub_size_t size; >char *buf; > > - file = grub_file_open (args[i]); >
