Re: [PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN
On Tue, Oct 8, 2019 at 9:55 PM Javier Martinez Canillas wrote: > Signed-off-by: Javier Martinez Canillas > Acked-by: Laszlo Ersek Acked-by: Matthew Garrett
Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
On Tue, Aug 27, 2019 at 6:42 AM Jarkko Sakkinen wrote: > > On Tue, Aug 27, 2019 at 02:03:44PM +0300, Jarkko Sakkinen wrote: > > > Jarkko, these two should probably go to 5.3 if possible - I > > > independently had a report of a system hitting this issue last week > > > (Intel apparently put a surprising amount of data in the event logs on > > > the NUCs). > > > > OK, I can try to push them. I'll do PR today. > > Ard, how do you wish these to be managed? > > I'm asking this because: > > 1. Neither patch was CC'd to linux-integrity. > 2. Neither patch has your tags ATM. Feel free to add my tags, but I don't think it's important.
Re: [PATCH 2/2] efi+tpm: don't traverse an event log with no events
On Mon, Aug 26, 2019 at 9:30 AM Jarkko Sakkinen wrote: > > On Mon, Aug 26, 2019 at 11:30:28AM -0400, Peter Jones wrote: > > When there are no entries to put into the final event log, some machines > > will return the template they would have populated anyway. In this case > > the nr_events field is 0, but the rest of the log is just garbage. > > > > This patch stops us from trying to iterate the table with > > __calc_tpm2_event_size() when the number of events in the table is 0. > > > > Signed-off-by: Peter Jones > > Tested-by: Lyude Paul > > --- > > drivers/firmware/efi/tpm.c | 14 +- > > 1 file changed, 9 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c > > index 1d3f5ca3eaa..be51ed17c6e 100644 > > --- a/drivers/firmware/efi/tpm.c > > +++ b/drivers/firmware/efi/tpm.c > > @@ -75,11 +75,15 @@ int __init efi_tpm_eventlog_init(void) > > goto out; > > } > > > > - tbl_size = tpm2_calc_event_log_size((void *)efi.tpm_final_log > > - + sizeof(final_tbl->version) > > - + sizeof(final_tbl->nr_events), > > - final_tbl->nr_events, > > - log_tbl->log); > > + tbl_size = 0; > > + if (final_tbl->nr_events != 0) { > > + void *events = (void *)efi.tpm_final_log > > + + sizeof(final_tbl->version) > > + + sizeof(final_tbl->nr_events); > > + tbl_size = tpm2_calc_event_log_size(events, > > + final_tbl->nr_events, > > + log_tbl->log); > > + } > > Reviewed-by: Jarkko Sakkinen Acked-by: Matthew Garrett
Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen wrote: > > On Mon, Aug 26, 2019 at 11:30:27AM -0400, Peter Jones wrote: > > Some machines generate a lot of event log entries. When we're > > iterating over them, the code removes the old mapping and adds a > > new one, so once we cross the page boundary we're unmapping the page > > with the count on it. Hilarity ensues. > > > > This patch keeps the info from the header in local variables so we don't > > need to access that page again or keep track of if it's mapped. > > > > Signed-off-by: Peter Jones > > Tested-by: Lyude Paul > > Reviewed-by: Jarkko Sakkinen Acked-by: Matthew Garrett Jarkko, these two should probably go to 5.3 if possible - I independently had a report of a system hitting this issue last week (Intel apparently put a surprising amount of data in the event logs on the NUCs).
[PATCH V40 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Acked-by: Ard Biesheuvel Reviewed-by: Kees Cook Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org Signed-off-by: James Morris --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 4b7cf7bc0ded..5f98374f77f4 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -241,6 +242,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.23.0.rc1.153.gdeed80330f-goog
[PATCH V38 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Acked-by: Ard Biesheuvel Reviewed-by: Kees Cook Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index ad3b1f4866b3..776f479e5499 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -242,6 +243,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.770.g0f2c4a37fd-goog
Re: [PATCH 5.3 regression fix] efi-stub: Fix get_efi_config_table on mixed-mode setups
On Wed, Aug 7, 2019 at 2:59 PM Hans de Goede wrote: > > Fix get_efi_config_table using the wrong structs when booting a > 64 bit kernel on 32 bit firmware. > > Cc: Matthew Garrett > Cc: Ard Biesheuvel > Cc: Jarkko Sakkinen > Fixes: 82d736ac56d7 ("Abstract out support for locating an EFI config table") > Signed-off-by: Hans de Goede Acked-By: Matthew Garrett Good catch. I think fixing this is preferable to reverting - the duplicate events are visible to userland, so there's a risk that apps will end up depending on them if there's a release that behaves that way. Presumably mixed mode isn't a thing on ARM?
[PATCH V37 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Acked-by: Ard Biesheuvel Reviewed-by: Kees Cook Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index ad3b1f4866b3..776f479e5499 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -242,6 +243,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.770.g0f2c4a37fd-goog
[PATCH V36 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Acked-by: Ard Biesheuvel Reviewed-by: Kees Cook Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index ad3b1f4866b3..776f479e5499 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -242,6 +243,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.510.g264f2c817a-goog
[PATCH V35 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Acked-by: Ard Biesheuvel Reviewed-by: Kees Cook Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index ad3b1f4866b3..776f479e5499 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -242,6 +243,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.510.g264f2c817a-goog
[PATCH V34 28/29] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 55b77c576c42..9f92a013ab27 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -31,6 +31,7 @@ #include #include #include +#include #include @@ -242,6 +243,11 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + int ret = security_locked_down(LOCKDOWN_ACPI_TABLES); + + if (ret) + return ret; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.410.gd8fdbe21b5-goog
[PATCH V33 30/30] efi: Restrict efivar_ssdt_load when the kernel is locked down
efivar_ssdt_load allows the kernel to import arbitrary ACPI code from an EFI variable, which gives arbitrary code execution in ring 0. Prevent that when the kernel is locked down. Signed-off-by: Matthew Garrett Cc: Ard Biesheuvel Cc: linux-efi@vger.kernel.org --- drivers/firmware/efi/efi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 55b77c576c42..a9ea649e0512 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -31,6 +31,7 @@ #include #include #include +#include #include @@ -242,6 +243,9 @@ static void generic_ops_unregister(void) static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata; static int __init efivar_ssdt_setup(char *str) { + if (security_is_locked_down(LOCKDOWN_ACPI_TABLES)) + return -EPERM; + if (strlen(str) < sizeof(efivar_ssdt)) memcpy(efivar_ssdt, str, strlen(str)); else -- 2.22.0.410.gd8fdbe21b5-goog
Re: [PATCH] drivers: firmware: efi: fix gcc warning -Wint-conversion
On Thu, Jun 20, 2019 at 2:37 PM Jarkko Sakkinen wrote: > Right! OK, I squashed just the fix to the earlier patch. Master and > next are updated. Can you take a peek of [1] and see if it looks > legit given all the fuzz around these changes? Then I'm confident > enough to do the 5.3 PR. All looks good to me. Thanks!
Re: [PATCH] drivers: firmware: efi: fix gcc warning -Wint-conversion
On Wed, Jun 19, 2019 at 2:55 AM Ard Biesheuvel wrote: > > (+ Jarkko, tpmdd, Matthew) > > On Sat, 15 Jun 2019 at 06:02, Hariprasad Kelam > wrote: > > > > This patch fixes below warning > > > > drivers/firmware/efi/tpm.c:78:38: warning: passing argument 1 of > > ‘tpm2_calc_event_log_size’ makes pointer from integer without a cast > > [-Wint-conversion] > > > > Signed-off-by: Hariprasad Kelam > > I think we already have a fix queued for this, no? It looks like I fixed this in "Don't duplicate events from the final event log in the TCG2 log" rather than a separate patch - I'm fine merging this, based on Jarkko's preferences.
Re: [PATCH V2 1/2] Abstract out support for locating an EFI config table
On Mon, Jun 10, 2019 at 9:58 AM Jarkko Sakkinen wrote: > > On Fri, Jun 07, 2019 at 01:51:46PM -0700, Matthew Garrett wrote: > > We want to grab a pointer to the TPM final events table, so abstract out > > the existing code for finding an FDT table and make it generic. > > > > Signed-off-by: Matthew Garrett > > Just to clarify are these extensions to what you did before and not > something that needs be squashed your commits pipelined for v5.3? Correct - they handle a corner case. Ideally they'd hit 5.3 as well, but if you'd prefer I'm ok waiting.
[PATCH V2 2/2] tpm: Don't duplicate events from the final event log in the TCG2 log
After the first call to GetEventLog() on UEFI systems using the TCG2 crypto agile log format, any further log events (other than those triggered by ExitBootServices()) will be logged in both the main log and also in the Final Events Log. While the kernel only calls GetEventLog() immediately before ExitBootServices(), we can't control whether earlier parts of the boot process have done so. This will result in log entries that exist in both logs, and so the current approach of simply appending the Final Event Log to the main log will result in events being duplicated. We can avoid this problem by looking at the size of the Final Event Log just before we call ExitBootServices() and exporting this to the main kernel. The kernel can then skip over all events that occured before ExitBootServices() and only append events that were not also logged to the main log. Signed-off-by: Matthew Garrett Reported-by: Joe Richey Suggested-by: Joe Richey --- drivers/char/tpm/eventlog/efi.c| 11 ++- drivers/firmware/efi/libstub/tpm.c | 30 ++ drivers/firmware/efi/tpm.c | 2 +- include/linux/efi.h| 1 + 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 9179cf6bdee9..be6540f2cb3d 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -80,6 +80,8 @@ int tpm_read_log_efi(struct tpm_chip *chip) goto out; } + efi_tpm_final_log_size -= log_tbl->final_events_preboot_size; + tmp = krealloc(log->bios_event_log, log_size + efi_tpm_final_log_size, GFP_KERNEL); @@ -90,8 +92,15 @@ int tpm_read_log_efi(struct tpm_chip *chip) } log->bios_event_log = tmp; + + /* +* Copy any of the final events log that didn't also end up in the +* main log. Events can be logged in both if events are generated +* between GetEventLog() and ExitBootServices(). +*/ memcpy((void *)log->bios_event_log + log_size, - final_tbl->events, efi_tpm_final_log_size); + final_tbl->events + log_tbl->final_events_preboot_size, + efi_tpm_final_log_size); log->bios_event_log_end = log->bios_event_log + log_size + efi_tpm_final_log_size; diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index 6b3b507a54eb..eb9af83e4d59 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -64,11 +64,13 @@ void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) efi_status_t status; efi_physical_addr_t log_location = 0, log_last_entry = 0; struct linux_efi_tpm_eventlog *log_tbl = NULL; + struct efi_tcg2_final_events_table *final_events_table; unsigned long first_entry_addr, last_entry_addr; size_t log_size, last_entry_size; efi_bool_t truncated; int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; void *tcg2_protocol = NULL; + int final_events_size = 0; status = efi_call_early(locate_protocol, &tcg2_guid, NULL, &tcg2_protocol); @@ -134,8 +136,36 @@ void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) return; } + /* +* Figure out whether any events have already been logged to the +* final events structure, and if so how much space they take up +*/ + final_events_table = get_efi_config_table(sys_table_arg, + LINUX_EFI_TPM_FINAL_LOG_GUID); + if (final_events_table && final_events_table->nr_events) { + struct tcg_pcr_event2_head *header; + int offset; + void *data; + int event_size; + int i = final_events_table->nr_events; + + data = (void *)final_events_table; + offset = sizeof(final_events_table->version) + + sizeof(final_events_table->nr_events); + + while (i > 0) { + header = data + offset + final_events_size; + event_size = __calc_tpm2_event_size(header, + (void *)(long)log_location, + false); + final_events_size += event_size; + i--; + } + } + memset(log_tbl, 0, sizeof(*log_tbl) + log_size); log_tbl->size = log_size; + log_tbl->final_events_preboot_size = final_events_size; log_tbl->version = version; memcpy(log_tbl->log, (void *) first_entry_addr, log_size); diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.
[PATCH V2 1/2] Abstract out support for locating an EFI config table
We want to grab a pointer to the TPM final events table, so abstract out the existing code for finding an FDT table and make it generic. Signed-off-by: Matthew Garrett --- .../firmware/efi/libstub/efi-stub-helper.c| 15 +++ drivers/firmware/efi/libstub/efistub.h| 2 ++ drivers/firmware/efi/libstub/fdt.c| 27 +++ 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e4610e72b78f..1db780c0f07b 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -926,3 +926,18 @@ efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg, fail: return status; } + +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid) +{ + efi_config_table_t *tables = (efi_config_table_t *)sys_table->tables; + int i; + + for (i = 0; i < sys_table->nr_tables; i++) { + if (efi_guidcmp(tables[i].guid, guid) != 0) + continue; + + return (void *)tables[i].table; + } + + return NULL; +} diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 1b1dfcaa6fb9..7f1556fd867d 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -65,6 +65,8 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg); efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg); +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid); + /* Helper macros for the usual case of using simple C variables: */ #ifndef fdt_setprop_inplace_var #define fdt_setprop_inplace_var(fdt, node_offset, name, var) \ diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 5440ba17a1c5..0bf0190917e0 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -363,26 +363,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size) { - efi_guid_t fdt_guid = DEVICE_TREE_GUID; - efi_config_table_t *tables; - int i; + void *fdt; - tables = (efi_config_table_t *)sys_table->tables; + fdt = get_efi_config_table(sys_table, DEVICE_TREE_GUID); - for (i = 0; i < sys_table->nr_tables; i++) { - void *fdt; + if (!fdt) + return NULL; - if (efi_guidcmp(tables[i].guid, fdt_guid) != 0) - continue; - - fdt = (void *)tables[i].table; - if (fdt_check_header(fdt) != 0) { - pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n"); - return NULL; - } - *fdt_size = fdt_totalsize(fdt); - return fdt; + if (fdt_check_header(fdt) != 0) { + pr_efi_err(sys_table, "Invalid header detected on UEFI supplied FDT, ignoring ...\n"); + return NULL; } - - return NULL; + *fdt_size = fdt_totalsize(fdt); + return fdt; } -- 2.22.0.rc2.383.gf4fbbf30c2-goog
Re: [PATCH] efi: Fix TPM code build failure on ARM
On Thu, Jun 6, 2019 at 4:39 AM Ard Biesheuvel wrote: > > On Wed, 5 Jun 2019 at 20:11, Matthew Garrett > wrote: > > > > asm/early_ioremap.h needs to be #included before tpm_eventlog.h in order > > to ensure that early_memremap is available. > > > > Doesn't that make it tpm_eventlog.h's job to #include it? tpm_eventlog.h doesn't use early_memremap directly, it's expanded from the macros declared in tpm.c .
[PATCH] efi: Fix TPM code build failure on ARM
asm/early_ioremap.h needs to be #included before tpm_eventlog.h in order to ensure that early_memremap is available. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/tpm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 0bdceb5913aa..1d3f5ca3eaaf 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -7,13 +7,12 @@ #define TPM_MEMREMAP(start, size) early_memremap(start, size) #define TPM_MEMUNMAP(start, size) early_memunmap(start, size) +#include #include #include #include #include -#include - int efi_tpm_final_log_size; EXPORT_SYMBOL(efi_tpm_final_log_size); -- 2.22.0.rc1.311.g5d7573a151-goog
[PATCH V2] tpm: Don't duplicate events from the final event log in the TCG2 log
After the first call to GetEventLog() on UEFI systems using the TCG2 crypto agile log format, any further log events (other than those triggered by ExitBootServices()) will be logged in both the main log and also in the Final Events Log. While the kernel only calls GetEventLog() immediately before ExitBootServices(), we can't control whether earlier parts of the boot process have done so. This will result in log entries that exist in both logs, and so the current approach of simply appending the Final Event Log to the main log will result in events being duplicated. We can avoid this problem by looking at the size of the Final Event Log just before we call ExitBootServices() and exporting this to the main kernel. The kernel can then skip over all events that occured before ExitBootServices() and only append events that were not also logged to the main log. Signed-off-by: Matthew Garrett Reported-by: Joe Richey Suggested-by: Joe Richey --- Unmodified other than changing the name of final_events_early_size to final_events_preboot_size. drivers/char/tpm/eventlog/efi.c | 11 ++- .../firmware/efi/libstub/efi-stub-helper.c| 15 ++ drivers/firmware/efi/libstub/efistub.h| 2 ++ drivers/firmware/efi/libstub/fdt.c| 27 ++--- drivers/firmware/efi/libstub/tpm.c| 30 +++ drivers/firmware/efi/tpm.c| 2 +- include/linux/efi.h | 1 + 7 files changed, 68 insertions(+), 20 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 9179cf6bdee9..be6540f2cb3d 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -80,6 +80,8 @@ int tpm_read_log_efi(struct tpm_chip *chip) goto out; } + efi_tpm_final_log_size -= log_tbl->final_events_preboot_size; + tmp = krealloc(log->bios_event_log, log_size + efi_tpm_final_log_size, GFP_KERNEL); @@ -90,8 +92,15 @@ int tpm_read_log_efi(struct tpm_chip *chip) } log->bios_event_log = tmp; + + /* +* Copy any of the final events log that didn't also end up in the +* main log. Events can be logged in both if events are generated +* between GetEventLog() and ExitBootServices(). +*/ memcpy((void *)log->bios_event_log + log_size, - final_tbl->events, efi_tpm_final_log_size); + final_tbl->events + log_tbl->final_events_preboot_size, + efi_tpm_final_log_size); log->bios_event_log_end = log->bios_event_log + log_size + efi_tpm_final_log_size; diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e4610e72b78f..1db780c0f07b 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -926,3 +926,18 @@ efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg, fail: return status; } + +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid) +{ + efi_config_table_t *tables = (efi_config_table_t *)sys_table->tables; + int i; + + for (i = 0; i < sys_table->nr_tables; i++) { + if (efi_guidcmp(tables[i].guid, guid) != 0) + continue; + + return (void *)tables[i].table; + } + + return NULL; +} diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 1b1dfcaa6fb9..7f1556fd867d 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -65,6 +65,8 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg); efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg); +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid); + /* Helper macros for the usual case of using simple C variables: */ #ifndef fdt_setprop_inplace_var #define fdt_setprop_inplace_var(fdt, node_offset, name, var) \ diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 5440ba17a1c5..0bf0190917e0 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -363,26 +363,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size) { - efi_guid_t fdt_guid = DEVICE_TREE_GUID; - efi_config_table_t *tables; - int i; + void *fdt; - tables = (efi_config_table_t *)sys_table->tables; + fdt = get_efi_config_table(sys_table, DEVICE_TREE_GUID); - for (i = 0; i < sys_table->nr_tables; i++) { - void *fdt; + if (!fdt) + return NULL; - if (efi_guidcmp(tables[i]
[PATCH] tpm: Don't duplicate events from the final event log in the TCG2 log
After the first call to GetEventLog() on UEFI systems using the TCG2 crypto agile log format, any further log events (other than those triggered by ExitBootServices()) will be logged in both the main log and also in the Final Events Log. While the kernel only calls GetEventLog() immediately before ExitBootServices(), we can't control whether earlier parts of the boot process have done so. This will result in log entries that exist in both logs, and so the current approach of simply appending the Final Event Log to the main log will result in events being duplicated. We can avoid this problem by looking at the size of the Final Event Log just before we call ExitBootServices() and exporting this to the main kernel. The kernel can then skip over all events that occured before ExitBootServices() and only append events that were not also logged to the main log. Signed-off-by: Matthew Garrett Reported-by: Joe Richey Suggested-by: Joe Richey --- drivers/char/tpm/eventlog/efi.c | 11 ++- .../firmware/efi/libstub/efi-stub-helper.c| 15 ++ drivers/firmware/efi/libstub/efistub.h| 2 ++ drivers/firmware/efi/libstub/fdt.c| 27 ++--- drivers/firmware/efi/libstub/tpm.c| 30 +++ drivers/firmware/efi/tpm.c| 2 +- include/linux/efi.h | 1 + 7 files changed, 68 insertions(+), 20 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 9179cf6bdee9..06b7fc99aa4a 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -80,6 +80,8 @@ int tpm_read_log_efi(struct tpm_chip *chip) goto out; } + efi_tpm_final_log_size -= log_tbl->final_events_early_size; + tmp = krealloc(log->bios_event_log, log_size + efi_tpm_final_log_size, GFP_KERNEL); @@ -90,8 +92,15 @@ int tpm_read_log_efi(struct tpm_chip *chip) } log->bios_event_log = tmp; + + /* +* Copy any of the final events log that didn't also end up in the +* main log. Events can be logged in both if events are generated +* between GetEventLog() and ExitBootServices(). +*/ memcpy((void *)log->bios_event_log + log_size, - final_tbl->events, efi_tpm_final_log_size); + final_tbl->events + log_tbl->final_events_early_size, + efi_tpm_final_log_size); log->bios_event_log_end = log->bios_event_log + log_size + efi_tpm_final_log_size; diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e4610e72b78f..1db780c0f07b 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -926,3 +926,18 @@ efi_status_t efi_exit_boot_services(efi_system_table_t *sys_table_arg, fail: return status; } + +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid) +{ + efi_config_table_t *tables = (efi_config_table_t *)sys_table->tables; + int i; + + for (i = 0; i < sys_table->nr_tables; i++) { + if (efi_guidcmp(tables[i].guid, guid) != 0) + continue; + + return (void *)tables[i].table; + } + + return NULL; +} diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 1b1dfcaa6fb9..7f1556fd867d 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -65,6 +65,8 @@ efi_status_t check_platform_features(efi_system_table_t *sys_table_arg); efi_status_t efi_random_get_seed(efi_system_table_t *sys_table_arg); +void *get_efi_config_table(efi_system_table_t *sys_table, efi_guid_t guid); + /* Helper macros for the usual case of using simple C variables: */ #ifndef fdt_setprop_inplace_var #define fdt_setprop_inplace_var(fdt, node_offset, name, var) \ diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c index 5440ba17a1c5..0bf0190917e0 100644 --- a/drivers/firmware/efi/libstub/fdt.c +++ b/drivers/firmware/efi/libstub/fdt.c @@ -363,26 +363,17 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table, void *get_fdt(efi_system_table_t *sys_table, unsigned long *fdt_size) { - efi_guid_t fdt_guid = DEVICE_TREE_GUID; - efi_config_table_t *tables; - int i; + void *fdt; - tables = (efi_config_table_t *)sys_table->tables; + fdt = get_efi_config_table(sys_table, DEVICE_TREE_GUID); - for (i = 0; i < sys_table->nr_tables; i++) { - void *fdt; + if (!fdt) + return NULL; - if (efi_guidcmp(tables[i].guid, fdt_guid) != 0) - continue; - - fdt = (void *)tables[i
Re: [Bug 203761] New: efivar_ssdt_iter is subject to stack corruption when the input name_size is 0
On Fri, May 31, 2019 at 2:03 AM Ard Biesheuvel wrote: > > The input of name_size is signed long, gets compared against an unsigned > > long > > of a fixed size, then stored as a signed int (this is mostly okay because of > > the known max size), but it then gets passed to a function takes unsigned > > long > > without checking the range. > > > > Here, the input name_size is 0, limit also is 0, but limit - 1 = -1, and > > then > > casts to ULONGMAX to ucs2_as_utf8 and corrupts the stack storage with a > > size of > > only EFIVAR_SSDT_NAME_MAX. This is a legitimate bug, but anyone in a position to trigger this is also in a position to inject an arbitrary SSDT which then means arbitrary code execution in the kernel, so I don't think there's any security-relevant impact.
[PATCH V7 4/4] efi: Attempt to get the TCG2 event log in the boot stub
From: Matthew Garrett Right now we only attempt to obtain the SHA1-only event log. The protocol also supports a crypto agile log format, which contains digests for all algorithms in use. Attempt to obtain this first, and fall back to obtaining the older format if the system doesn't support it. This is lightly complicated by the event sizes being variable (as we don't know in advance which algorithms are in use), and the interface giving us back a pointer to the start of the final entry rather than a pointer to the end of the log - as a result, we need to parse the final entry to figure out its length in order to know how much data to copy up to the OS. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/libstub/tpm.c | 50 -- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index 5bd04f75d8d6..6b3b507a54eb 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -57,7 +57,7 @@ void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) #endif -static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) +void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) { efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID; efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; @@ -67,6 +67,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) unsigned long first_entry_addr, last_entry_addr; size_t log_size, last_entry_size; efi_bool_t truncated; + int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; void *tcg2_protocol = NULL; status = efi_call_early(locate_protocol, &tcg2_guid, NULL, @@ -74,14 +75,20 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) if (status != EFI_SUCCESS) return; - status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol, - EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2, - &log_location, &log_last_entry, &truncated); - if (status != EFI_SUCCESS) - return; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + + if (status != EFI_SUCCESS || !log_location) { + version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + if (status != EFI_SUCCESS || !log_location) + return; + + } - if (!log_location) - return; first_entry_addr = (unsigned long) log_location; /* @@ -96,8 +103,23 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) * We need to calculate its size to deduce the full size of * the logs. */ - last_entry_size = sizeof(struct tcpa_event) + - ((struct tcpa_event *) last_entry_addr)->event_size; + if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { + /* +* The TCG2 log format has variable length entries, +* and the information to decode the hash algorithms +* back into a size is contained in the first entry - +* pass a pointer to the final entry (to calculate its +* size) and the first entry (so we know how long each +* digest is) +*/ + last_entry_size = + __calc_tpm2_event_size((void *)last_entry_addr, + (void *)(long)log_location, + false); + } else { + last_entry_size = sizeof(struct tcpa_event) + + ((struct tcpa_event *) last_entry_addr)->event_size; + } log_size = log_last_entry - log_location + last_entry_size; } @@ -114,7 +136,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) memset(log_tbl, 0, sizeof(*log_tbl) + log_size); log_tbl->size = log_size; - log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + log_tbl->version = version; memcpy(log_tbl->log, (void *) first_entry_addr, log_size); status = efi_call_early(install_configuration_table, @@ -126,9 +148,3 @@ static void efi_ret
[PATCH V7 0/4] Add support for crypto agile logs
Identical to previous version except without the KSAN workaround - Ard has a better solution for that.
[PATCH V7 3/4] tpm: Append the final event log to the TPM event log
From: Matthew Garrett Any events that are logged after GetEventsLog() is called are logged to the EFI Final Events table. These events are defined as being in the crypto agile log format, so we can just append them directly to the existing log if it's in the same format. In theory we can also construct old-style SHA1 log entries for devices that only return logs in that format, but EDK2 doesn't generate the final event log in that case so it doesn't seem worth it at the moment. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/efi.c | 50 - 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 3e673ab22cb4..9179cf6bdee9 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -21,10 +21,13 @@ int tpm_read_log_efi(struct tpm_chip *chip) { + struct efi_tcg2_final_events_table *final_tbl = NULL; struct linux_efi_tpm_eventlog *log_tbl; struct tpm_bios_log *log; u32 log_size; u8 tpm_log_version; + void *tmp; + int ret; if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) return -ENODEV; @@ -52,15 +55,48 @@ int tpm_read_log_efi(struct tpm_chip *chip) /* malloc EventLog space */ log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); - if (!log->bios_event_log) - goto err_memunmap; - log->bios_event_log_end = log->bios_event_log + log_size; + if (!log->bios_event_log) { + ret = -ENOMEM; + goto out; + } + log->bios_event_log_end = log->bios_event_log + log_size; tpm_log_version = log_tbl->version; - memunmap(log_tbl); - return tpm_log_version; -err_memunmap: + ret = tpm_log_version; + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR || + efi_tpm_final_log_size == 0 || + tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) + goto out; + + final_tbl = memremap(efi.tpm_final_log, +sizeof(*final_tbl) + efi_tpm_final_log_size, +MEMREMAP_WB); + if (!final_tbl) { + pr_err("Could not map UEFI TPM final log\n"); + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + tmp = krealloc(log->bios_event_log, + log_size + efi_tpm_final_log_size, + GFP_KERNEL); + if (!tmp) { + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + log->bios_event_log = tmp; + memcpy((void *)log->bios_event_log + log_size, + final_tbl->events, efi_tpm_final_log_size); + log->bios_event_log_end = log->bios_event_log + + log_size + efi_tpm_final_log_size; + +out: + memunmap(final_tbl); memunmap(log_tbl); - return -ENOMEM; + return ret; } -- 2.21.0.1020.gf2820cf01a-goog
[PATCH V7 1/4] tpm: Abstract crypto agile event size calculations
From: Matthew Garrett We need to calculate the size of crypto agile events in multiple locations, including in the EFI boot stub. The easiest way to do this is to put it in a header file as an inline and leave a wrapper to ensure we don't end up with multiple copies of it embedded in the existing code. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 47 +- include/linux/tpm_eventlog.h | 68 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index f824563fc28d..1a977bdd3bd2 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,52 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - struct tcg_efi_specid_event_head *efispecid; - struct tcg_event_field *event_field; - void *marker; - void *marker_start; - u32 halg_size; - size_t size; - u16 halg; - int i; - int j; - - marker = event; - marker_start = marker; - marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) - + sizeof(event->count); - - efispecid = (struct tcg_efi_specid_event_head *)event_header->event; - - /* Check if event is malformed. */ - if (event->count > efispecid->num_algs) - return 0; - - for (i = 0; i < event->count; i++) { - halg_size = sizeof(event->digests[i].alg_id); - memcpy(&halg, marker, halg_size); - marker = marker + halg_size; - for (j = 0; j < efispecid->num_algs; j++) { - if (halg == efispecid->digest_sizes[j].alg_id) { - marker += - efispecid->digest_sizes[j].digest_size; - break; - } - } - /* Algorithm without known length. Such event is unparseable. */ - if (j == efispecid->num_algs) - return 0; - } - - event_field = (struct tcg_event_field *)marker; - marker = marker + sizeof(event_field->event_size) - + event_field->event_size; - size = marker - marker_start; - - if ((event->event_type == 0) && (event_field->event_size == 0)) - return 0; - - return size; + return __calc_tpm2_event_size(event, event_header); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index 81519f163211..6a86144e13f1 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -112,4 +112,72 @@ struct tcg_pcr_event2_head { struct tpm_digest digests[]; } __packed; +/** + * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry + * @event:Pointer to the event whose size should be calculated + * @event_header: Pointer to the initial event containing the digest lengths + * + * The TPM2 event log format can contain multiple digests corresponding to + * separate PCR banks, and also contains a variable length of the data that + * was measured. This requires knowledge of how long each digest type is, + * and this information is contained within the first event in the log. + * + * We calculate the length by examining the number of events, and then looking + * at each event in turn to determine how much space is used for events in + * total. Once we've done this we know the offset of the data length field, + * and can calculate the total size of the event. + * + * Return: size of the event on success, <0 on failure + */ + +static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, +struct tcg_pcr_event *event_header) +{ + struct tcg_efi_specid_event_head *efispecid; + struct tcg_event_field *event_field; + void *marker; + void *marker_start; + u32 halg_size; + size_t size; + u16 halg; + int i; + int j; + + marker = event; + marker_start = marker; + marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) + + sizeof(event->count); + + efispecid = (struct tcg_efi_specid_event_head *)event_header->event; + + /* Check if event is malformed. */ + if (event->count > efispecid->num_algs) + return 0; + + for (i = 0; i < event->count; i++) { + halg_size = sizeof(event->digests[i].alg_id); + memcpy(&halg, marker, halg_size); + marker = marker + halg_size; + for (j = 0; j < efispecid->num_al
[PATCH V7 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett UEFI systems provide a boot services protocol for obtaining the TPM event log, but this is unusable after ExitBootServices() is called. Unfortunately ExitBootServices() itself triggers additional TPM events that then can't be obtained using this protocol. The platform provides a mechanism for the OS to obtain these events by recording them to a separate UEFI configuration table which the OS can then map. Unfortunately this table isn't self describing in terms of providing its length, so we need to parse the events inside it to figure out how long it is. Since the table isn't mapped at this point, we need to extend the length calculation function to be able to map the event as it goes along. (Fixes by Bartosz Szczepanek ) Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 2 +- drivers/firmware/efi/efi.c | 2 + drivers/firmware/efi/tpm.c | 62 ++- include/linux/efi.h | 9 +++ include/linux/tpm_eventlog.h | 102 --- 5 files changed, 164 insertions(+), 13 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index 1a977bdd3bd2..de1d9f7e5a92 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,7 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - return __calc_tpm2_event_size(event, event_header); + return __calc_tpm2_event_size(event, event_header, false); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 55b77c576c42..6b11c41e0575 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -53,6 +53,7 @@ struct efi __read_mostly efi = { .mem_attr_table = EFI_INVALID_TABLE_ADDR, .rng_seed = EFI_INVALID_TABLE_ADDR, .tpm_log= EFI_INVALID_TABLE_ADDR, + .tpm_final_log = EFI_INVALID_TABLE_ADDR, .mem_reserve= EFI_INVALID_TABLE_ADDR, }; EXPORT_SYMBOL(efi); @@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] = { {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table}, {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed}, {LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log}, + {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", &efi.tpm_final_log}, {LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve}, {NULL_GUID, NULL, NULL}, }; diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 3a689b40ccc0..2c912ea08166 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -4,34 +4,90 @@ * Thiebaud Weksteen */ +#define TPM_MEMREMAP(start, size) early_memremap(start, size) +#define TPM_MEMUNMAP(start, size) early_memunmap(start, size) + #include #include #include +#include #include +int efi_tpm_final_log_size; +EXPORT_SYMBOL(efi_tpm_final_log_size); + +static int tpm2_calc_event_log_size(void *data, int count, void *size_info) +{ + struct tcg_pcr_event2_head *header; + int event_size, size = 0; + + while (count > 0) { + header = data + size; + event_size = __calc_tpm2_event_size(header, size_info, true); + if (event_size == 0) + return -1; + size += event_size; + count--; + } + + return size; +} + /* * Reserve the memory associated with the TPM Event Log configuration table. */ int __init efi_tpm_eventlog_init(void) { struct linux_efi_tpm_eventlog *log_tbl; + struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; + int ret = 0; - if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { + /* +* We can't calculate the size of the final events without the +* first entry in the TPM log, so bail here. +*/ return 0; + } log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl)); if (!log_tbl) { pr_err("Failed to map TPM Event Log table @ 0x%lx\n", - efi.tpm_log); + efi.tpm_log); efi.tpm_log = EFI_INVALID_TABLE_ADDR; return -ENOMEM; } tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) + goto out; + + final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tb
[PATCH V6 3/4] tpm: Append the final event log to the TPM event log
From: Matthew Garrett Any events that are logged after GetEventsLog() is called are logged to the EFI Final Events table. These events are defined as being in the crypto agile log format, so we can just append them directly to the existing log if it's in the same format. In theory we can also construct old-style SHA1 log entries for devices that only return logs in that format, but EDK2 doesn't generate the final event log in that case so it doesn't seem worth it at the moment. Signed-off-by: Matthew Garrett Tested-by: Jarkko Sakkinen Reviewed-by: Jarkko Sakkinen Signed-off-by: Jarkko Sakkinen --- drivers/char/tpm/eventlog/efi.c | 50 - 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 3e673ab22cb4..9179cf6bdee9 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -21,10 +21,13 @@ int tpm_read_log_efi(struct tpm_chip *chip) { + struct efi_tcg2_final_events_table *final_tbl = NULL; struct linux_efi_tpm_eventlog *log_tbl; struct tpm_bios_log *log; u32 log_size; u8 tpm_log_version; + void *tmp; + int ret; if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) return -ENODEV; @@ -52,15 +55,48 @@ int tpm_read_log_efi(struct tpm_chip *chip) /* malloc EventLog space */ log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); - if (!log->bios_event_log) - goto err_memunmap; - log->bios_event_log_end = log->bios_event_log + log_size; + if (!log->bios_event_log) { + ret = -ENOMEM; + goto out; + } + log->bios_event_log_end = log->bios_event_log + log_size; tpm_log_version = log_tbl->version; - memunmap(log_tbl); - return tpm_log_version; -err_memunmap: + ret = tpm_log_version; + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR || + efi_tpm_final_log_size == 0 || + tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) + goto out; + + final_tbl = memremap(efi.tpm_final_log, +sizeof(*final_tbl) + efi_tpm_final_log_size, +MEMREMAP_WB); + if (!final_tbl) { + pr_err("Could not map UEFI TPM final log\n"); + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + tmp = krealloc(log->bios_event_log, + log_size + efi_tpm_final_log_size, + GFP_KERNEL); + if (!tmp) { + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + log->bios_event_log = tmp; + memcpy((void *)log->bios_event_log + log_size, + final_tbl->events, efi_tpm_final_log_size); + log->bios_event_log_end = log->bios_event_log + + log_size + efi_tpm_final_log_size; + +out: + memunmap(final_tbl); memunmap(log_tbl); - return -ENOMEM; + return ret; } -- 2.21.0.1020.gf2820cf01a-goog
[PATCH V6 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett UEFI systems provide a boot services protocol for obtaining the TPM event log, but this is unusable after ExitBootServices() is called. Unfortunately ExitBootServices() itself triggers additional TPM events that then can't be obtained using this protocol. The platform provides a mechanism for the OS to obtain these events by recording them to a separate UEFI configuration table which the OS can then map. Unfortunately this table isn't self describing in terms of providing its length, so we need to parse the events inside it to figure out how long it is. Since the table isn't mapped at this point, we need to extend the length calculation function to be able to map the event as it goes along. (Fixes by Bartosz Szczepanek ) Signed-off-by: Matthew Garrett Tested-by: Jarkko Sakkinen Reviewed-by: Jarkko Sakkinen Signed-off-by: Jarkko Sakkinen --- drivers/char/tpm/eventlog/tpm2.c | 2 +- drivers/firmware/efi/efi.c | 2 + drivers/firmware/efi/tpm.c | 62 ++- include/linux/efi.h | 9 +++ include/linux/tpm_eventlog.h | 102 --- 5 files changed, 164 insertions(+), 13 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index 1a977bdd3bd2..de1d9f7e5a92 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,7 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - return __calc_tpm2_event_size(event, event_header); + return __calc_tpm2_event_size(event, event_header, false); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 55b77c576c42..6b11c41e0575 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -53,6 +53,7 @@ struct efi __read_mostly efi = { .mem_attr_table = EFI_INVALID_TABLE_ADDR, .rng_seed = EFI_INVALID_TABLE_ADDR, .tpm_log= EFI_INVALID_TABLE_ADDR, + .tpm_final_log = EFI_INVALID_TABLE_ADDR, .mem_reserve= EFI_INVALID_TABLE_ADDR, }; EXPORT_SYMBOL(efi); @@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] = { {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table}, {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed}, {LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log}, + {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", &efi.tpm_final_log}, {LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve}, {NULL_GUID, NULL, NULL}, }; diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 3a689b40ccc0..2c912ea08166 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -4,34 +4,90 @@ * Thiebaud Weksteen */ +#define TPM_MEMREMAP(start, size) early_memremap(start, size) +#define TPM_MEMUNMAP(start, size) early_memunmap(start, size) + #include #include #include +#include #include +int efi_tpm_final_log_size; +EXPORT_SYMBOL(efi_tpm_final_log_size); + +static int tpm2_calc_event_log_size(void *data, int count, void *size_info) +{ + struct tcg_pcr_event2_head *header; + int event_size, size = 0; + + while (count > 0) { + header = data + size; + event_size = __calc_tpm2_event_size(header, size_info, true); + if (event_size == 0) + return -1; + size += event_size; + count--; + } + + return size; +} + /* * Reserve the memory associated with the TPM Event Log configuration table. */ int __init efi_tpm_eventlog_init(void) { struct linux_efi_tpm_eventlog *log_tbl; + struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; + int ret = 0; - if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { + /* +* We can't calculate the size of the final events without the +* first entry in the TPM log, so bail here. +*/ return 0; + } log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl)); if (!log_tbl) { pr_err("Failed to map TPM Event Log table @ 0x%lx\n", - efi.tpm_log); + efi.tpm_log); efi.tpm_log = EFI_INVALID_TABLE_ADDR; return -ENOMEM; } tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) + goto
[PATCH V6 4/4] efi: Attempt to get the TCG2 event log in the boot stub
From: Matthew Garrett Right now we only attempt to obtain the SHA1-only event log. The protocol also supports a crypto agile log format, which contains digests for all algorithms in use. Attempt to obtain this first, and fall back to obtaining the older format if the system doesn't support it. This is lightly complicated by the event sizes being variable (as we don't know in advance which algorithms are in use), and the interface giving us back a pointer to the start of the final entry rather than a pointer to the end of the log - as a result, we need to parse the final entry to figure out its length in order to know how much data to copy up to the OS. Signed-off-by: Matthew Garrett Tested-by: Jarkko Sakkinen Reviewed-by: Jarkko Sakkinen Signed-off-by: Jarkko Sakkinen --- drivers/firmware/efi/libstub/tpm.c | 57 -- 1 file changed, 39 insertions(+), 18 deletions(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index 5bd04f75d8d6..b3f30448e454 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -8,8 +8,13 @@ * Thiebaud Weksteen */ #include -#include #include +/* + * KASAN redefines memcpy() in a way that isn't available in the EFI stub. + * We need to include asm/efi.h before linux/tpm_eventlog.h in order to avoid + * the wrong memcpy() being referenced. + */ +#include #include "efistub.h" @@ -57,7 +62,7 @@ void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) #endif -static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) +void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) { efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID; efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; @@ -67,6 +72,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) unsigned long first_entry_addr, last_entry_addr; size_t log_size, last_entry_size; efi_bool_t truncated; + int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; void *tcg2_protocol = NULL; status = efi_call_early(locate_protocol, &tcg2_guid, NULL, @@ -74,14 +80,20 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) if (status != EFI_SUCCESS) return; - status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol, - EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2, - &log_location, &log_last_entry, &truncated); - if (status != EFI_SUCCESS) - return; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + + if (status != EFI_SUCCESS || !log_location) { + version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + if (status != EFI_SUCCESS || !log_location) + return; + + } - if (!log_location) - return; first_entry_addr = (unsigned long) log_location; /* @@ -96,8 +108,23 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) * We need to calculate its size to deduce the full size of * the logs. */ - last_entry_size = sizeof(struct tcpa_event) + - ((struct tcpa_event *) last_entry_addr)->event_size; + if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { + /* +* The TCG2 log format has variable length entries, +* and the information to decode the hash algorithms +* back into a size is contained in the first entry - +* pass a pointer to the final entry (to calculate its +* size) and the first entry (so we know how long each +* digest is) +*/ + last_entry_size = + __calc_tpm2_event_size((void *)last_entry_addr, + (void *)(long)log_location, + false); + } else { + last_entry_size = sizeof(struct tcpa_event) + + ((struct tcpa_event *) last_entry_addr)->event_size; + } log_size = log_last_entry - log_location + last_entry_size; } @@ -114,7 +141,7 @@ static void efi_retriev
[PATCH V6 1/4] tpm: Abstract crypto agile event size calculations
From: Matthew Garrett We need to calculate the size of crypto agile events in multiple locations, including in the EFI boot stub. The easiest way to do this is to put it in a header file as an inline and leave a wrapper to ensure we don't end up with multiple copies of it embedded in the existing code. Signed-off-by: Matthew Garrett Tested-by: Jarkko Sakkinen Reviewed-by: Jarkko Sakkinen Signed-off-by: Jarkko Sakkinen --- drivers/char/tpm/eventlog/tpm2.c | 47 +- include/linux/tpm_eventlog.h | 68 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index f824563fc28d..1a977bdd3bd2 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,52 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - struct tcg_efi_specid_event_head *efispecid; - struct tcg_event_field *event_field; - void *marker; - void *marker_start; - u32 halg_size; - size_t size; - u16 halg; - int i; - int j; - - marker = event; - marker_start = marker; - marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) - + sizeof(event->count); - - efispecid = (struct tcg_efi_specid_event_head *)event_header->event; - - /* Check if event is malformed. */ - if (event->count > efispecid->num_algs) - return 0; - - for (i = 0; i < event->count; i++) { - halg_size = sizeof(event->digests[i].alg_id); - memcpy(&halg, marker, halg_size); - marker = marker + halg_size; - for (j = 0; j < efispecid->num_algs; j++) { - if (halg == efispecid->digest_sizes[j].alg_id) { - marker += - efispecid->digest_sizes[j].digest_size; - break; - } - } - /* Algorithm without known length. Such event is unparseable. */ - if (j == efispecid->num_algs) - return 0; - } - - event_field = (struct tcg_event_field *)marker; - marker = marker + sizeof(event_field->event_size) - + event_field->event_size; - size = marker - marker_start; - - if ((event->event_type == 0) && (event_field->event_size == 0)) - return 0; - - return size; + return __calc_tpm2_event_size(event, event_header); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index 81519f163211..6a86144e13f1 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -112,4 +112,72 @@ struct tcg_pcr_event2_head { struct tpm_digest digests[]; } __packed; +/** + * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry + * @event:Pointer to the event whose size should be calculated + * @event_header: Pointer to the initial event containing the digest lengths + * + * The TPM2 event log format can contain multiple digests corresponding to + * separate PCR banks, and also contains a variable length of the data that + * was measured. This requires knowledge of how long each digest type is, + * and this information is contained within the first event in the log. + * + * We calculate the length by examining the number of events, and then looking + * at each event in turn to determine how much space is used for events in + * total. Once we've done this we know the offset of the data length field, + * and can calculate the total size of the event. + * + * Return: size of the event on success, <0 on failure + */ + +static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, +struct tcg_pcr_event *event_header) +{ + struct tcg_efi_specid_event_head *efispecid; + struct tcg_event_field *event_field; + void *marker; + void *marker_start; + u32 halg_size; + size_t size; + u16 halg; + int i; + int j; + + marker = event; + marker_start = marker; + marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) + + sizeof(event->count); + + efispecid = (struct tcg_efi_specid_event_head *)event_header->event; + + /* Check if event is malformed. */ + if (event->count > efispecid->num_algs) + return 0; + + for (i = 0; i < event->count; i++) { + halg_size = sizeof(event->digests[i].alg_id); + memcpy(&halg, mar
[PATCH V6 0/4] Add support for crypto agile TPM event logs
Updated with the fixes from Bartosz and the header fixes folded in. Bartosz, my machine still doesn't generate any final event log entries - are you able to give this a test and make sure it's good?
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
On Fri, May 10, 2019 at 2:31 PM Claudio Carvalho wrote: > On 4/10/19 2:36 PM, Matthew Garrett wrote: > > I don't see the benefit in attempting to maintain compatibility with > > existing tooling unless you're going to be *completely* compatible > > with existing tooling. That means supporting dbx and dbt. (snip) > In OS secure boot domain (work in progress): > - The skiroot container is verified as part of firmware secure boot. > - Skiroot uses UEFI-like secure variables (PK, KEK and db) to verify OS > kernels. Only X.509 certificates will be supported for these secure variables. You don't support hashes? If so, this isn't compatible with UEFI Secure Boot and we shouldn't try to make it look like UEFI Secure Boot. > How about dbx and dbt? > > The db keys will be used to verify only OS kernels via kexecs initiated by > petitboot. So we only need the dbx to revoke kernel images, either via > certs or hashes. Currently, the kernel loads certs and hashes from the dbx > to the system blacklist keyring. The revoked certs are checked during pkcs7 > signature verification and loading of keys. However, there doesn't appear > to be any verification against blacklisted hashes. Should kernel images be > revoked only by keys and not hashes? We tried to find published revoked > kernel lists but couldn't find any. How is kernel image revocation handled > in practice? Hash-based revocation is in active use in the UEFI world - to the best of my knowledge, all existing dbx entries are hashes with the exception of the invalidation of the Microsoft Windows 2010 CA. > Also, we didn't see the shim or kernel loading anything from dbt. dbt is currently only used for validation at the firmware level - the way grub and kernel signatures are currently managed means it doesn't make a huge amount of sense to use it in shim, but it would probably be reasonable to extend shim's validation to include dbt. > > So I do the following: > > > > 1) Boot > > 2) Extend the contents of db > > 3) Extend the contents of db again > > 4) Read back the contents of db through efivarfs > > 5) Reboot > > 6) Read back the contents of db through efivarfs > > > > Is what I see in (4) and (6) the same? Does it contain the values form > > both extensions? > > In (2) and (3) the extensions are added to the update queue, which is > processed only in (5) by firmware. So, in (4) you should see the db content > without the extensions. Ok, this is not what we expect from UEFI systems. I'm strongly against providing what looks like the same ABI on multiple platforms but carrying subtle differences between those platforms - it's guaranteed to break tooling in unexpected ways. > In (5), firmware (skiboot) will process the update queue. The extensions > will be applied only if *all* of them are valid and pass signature > verification. Only in this case should you be able to see the extensions in > (6). If any of the extensions fail, firmware will discard all of them, > clear the queue, and do the proper logging. I believe that this is also a violation of expectations. > > Why would the intermediate level organisations not just have entries > > in db? > > Because that seems to add more complexity than having three levels (PK, KEK > and db). > > Typically, the intermediate level organisations (or KEK) are used to > authorize new additions to db. However, if we also have them in the db, who > would authorize the new additions to db. If that would be the intermediate > level organisation entries now in the db, it seems we would need to > implement a mechanism to determine which entries are for authorizing new > additions and which are for kernel signature verification. If that would be > the PK, we'd be burdening the PK owner to sign every new db addition if the > platform is owned by a company that has intermediate level organizations. Ok, in this scenario I don't understand why you wouldn't just want the intermediates in PK. Or, put another way - if you have a business justification for three layers of hierarchy, what do you do when someone has a business justification for four? The three layer hierarchy represents the weirdness of the PC industry where you have Microsoft needing to be in KEK (because they need to be able to issue updates to machines from multiple vendors) but not wanting to be in PK (because vendors don't want Microsoft to have ultimate control over their systems). If it weren't for this conflict, we'd just have a two layer hierarchy, and if some other aspect of the market had evolved over time we'd have a four layer hierarchy. > > > The main reason we don't do it this way in UEFI is because we > > need to support db
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
Sorry, how about this one? I was confused by why I wasn't hitting this, but on closer examination it turns out that my system populates the final event log with 0 events which means we never hit this codepath :( diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 2ccaa6661aaf..db0fdaa9c666 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info) if (event_size == 0) return -1; size += event_size; + count--; } return size; @@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void) struct linux_efi_tpm_eventlog *log_tbl; struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; + int ret = 0; if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { /* @@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void) tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); - early_memunmap(log_tbl, sizeof(*log_tbl)); if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) - return 0; + goto out; final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); @@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void) pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n", efi.tpm_final_log); efi.tpm_final_log = EFI_INVALID_TABLE_ADDR; - return -ENOMEM; + ret = -ENOMEM; + goto out; } tbl_size = tpm2_calc_event_log_size(final_tbl->events, final_tbl->nr_events, - (void *)efi.tpm_log); + log_tbl->log); memblock_reserve((unsigned long)final_tbl, tbl_size + sizeof(*final_tbl)); early_memunmap(final_tbl, sizeof(*final_tbl)); efi_tpm_final_log_size = tbl_size; - return 0; +out: + early_memunmap(log_tbl, sizeof(*log_tbl)); + return ret; } diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index dccc97e6135c..190a33968a91 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, { struct tcg_efi_specid_event_head *efispecid; struct tcg_event_field *event_field; - void *mapping = NULL; int mapping_size; void *marker; void *marker_start; @@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the event header */ if (do_mapping) { mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, if (do_mapping) { early_memunmap(mapping, mapping_size); mapping_size = marker - marker_start + halg_size; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, if (do_mapping) { early_memunmap(mapping, mapping_size); mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, * we don't need to map it */ if (do_mapping) { - early_memunmap(marker_start, mapping_size); + early_memunmap(event, mapping_size); mapping_size += sizeof(event_field->event_size); - mapping = early_memremap((unsigned long)marker_start, - mapping_size); - if (!mapping) { + event = early_memremap((unsigned long)marker_start, + mapping_size); + if (!event) { size = 0; goto out; } @@ -257,8 +256,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, + event_field->event_size; size = marker - marker_start; - if ((event->event_type == 0) && (event_field->event_size == 0)) - size = 0; out: if (do_mapping) early_memunmap(mapping, mapping_size);
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel wrote: > > (+ Ingo) > > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote: > > > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek > > wrote: > > > > > > I may be a little late with this comment, but I've just tested these > > > patches on aarch64 platform (from the top of jjs/master) and got > > > kernel panic ("Unable to handle kernel read", full log at the end of > > > mail). I think there's problem with below call to > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is > > > passed as (void *) and never remapped: > > > > Yes, it looks like this is just broken. Can you try with the attached patch? > > I'm a bit uncomfortable with EFI code that is obviously broken and > untested being queued for the next merge window in another tree. The patchset was Cc:ed to linux-efi@. Is there anything else I should have done to ensure you picked it up rather than Jarkko?
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 2, 2019 at 1:32 AM Jarkko Sakkinen wrote: > > On Tue, Apr 30, 2019 at 03:07:09PM +0200, Bartosz Szczepanek wrote: > > I may be a little late with this comment, but I've just tested these > > patches on aarch64 platform (from the top of jjs/master) and got > > kernel panic ("Unable to handle kernel read", full log at the end of > > mail). I think there's problem with below call to > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is > > passed as (void *) and never remapped: > > Not late. This is not part of any PR yet. Thank you for the > feedback! > > Matthew, can you send an updated version of the whole patch set > with fixes to this issue and also reordering of the includes? Yes, I'll resend and let's do this again for 5.3.
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 12:51 PM Matthew Garrett wrote: > Yes, it looks like this is just broken. Can you try with the attached patch? Actually, please try this one. diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 2ccaa6661aaf..db0fdaa9c666 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info) if (event_size == 0) return -1; size += event_size; + count--; } return size; @@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void) struct linux_efi_tpm_eventlog *log_tbl; struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; + int ret = 0; if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { /* @@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void) tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); - early_memunmap(log_tbl, sizeof(*log_tbl)); if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) - return 0; + goto out; final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); @@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void) pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n", efi.tpm_final_log); efi.tpm_final_log = EFI_INVALID_TABLE_ADDR; - return -ENOMEM; + ret = -ENOMEM; + goto out; } tbl_size = tpm2_calc_event_log_size(final_tbl->events, final_tbl->nr_events, - (void *)efi.tpm_log); + log_tbl->log); memblock_reserve((unsigned long)final_tbl, tbl_size + sizeof(*final_tbl)); early_memunmap(final_tbl, sizeof(*final_tbl)); efi_tpm_final_log_size = tbl_size; - return 0; +out: + early_memunmap(log_tbl, sizeof(*log_tbl)); + return ret; } diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index dccc97e6135c..662410710ac0 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, { struct tcg_efi_specid_event_head *efispecid; struct tcg_event_field *event_field; - void *mapping = NULL; int mapping_size; void *marker; void *marker_start; @@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the event header */ if (do_mapping) { mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, if (do_mapping) { early_memunmap(mapping, mapping_size); mapping_size = marker - marker_start + halg_size; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, if (do_mapping) { early_memunmap(mapping, mapping_size); mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, + event = early_memremap((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, * we don't need to map it */ if (do_mapping) { - early_memunmap(marker_start, mapping_size); + early_memunmap(mapping, mapping_size); mapping_size += sizeof(event_field->event_size); - mapping = early_memremap((unsigned long)marker_start, - mapping_size); - if (!mapping) { + event = early_memremap((unsigned long)marker_start, + mapping_size); + if (!event) { size = 0; goto out; } @@ -257,8 +256,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, + event_field->event_size; size = marker - marker_start; - if ((event->event_type == 0) && (event_field->event_size == 0)) - size = 0; out: if (do_mapping) early_memunmap(mapping, mapping_size);
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek wrote: > > I may be a little late with this comment, but I've just tested these > patches on aarch64 platform (from the top of jjs/master) and got > kernel panic ("Unable to handle kernel read", full log at the end of > mail). I think there's problem with below call to > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is > passed as (void *) and never remapped: Yes, it looks like this is just broken. Can you try with the attached patch? diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index fe48150f06d1..9711bd34f8ae 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info) if (event_size == 0) return -1; size += event_size; + count--; } return size; @@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void) struct linux_efi_tpm_eventlog *log_tbl; struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; + int ret = 0; if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { /* @@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void) tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); - early_memunmap(log_tbl, sizeof(*log_tbl)); if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) - return 0; + goto out; final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); @@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void) pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n", efi.tpm_final_log); efi.tpm_final_log = EFI_INVALID_TABLE_ADDR; - return -ENOMEM; + ret = -ENOMEM; + goto out; } tbl_size = tpm2_calc_event_log_size(final_tbl->events, final_tbl->nr_events, - (void *)efi.tpm_log); + log_tbl->log); memblock_reserve((unsigned long)final_tbl, tbl_size + sizeof(*final_tbl)); early_memunmap(final_tbl, sizeof(*final_tbl)); efi_tpm_final_log_size = tbl_size; - return 0; +out: + early_memunmap(log_tbl, sizeof(*log_tbl)); + return ret; } diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index 0ca27bc053af..9cfbb14f54e6 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -161,7 +161,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, { struct tcg_efi_specid_event_head *efispecid; struct tcg_event_field *event_field; - void *mapping = NULL; int mapping_size; void *marker; void *marker_start; @@ -179,9 +178,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the event header */ if (do_mapping) { mapping_size = marker - marker_start; - mapping = TPM_MEMREMAP((unsigned long)marker_start, - mapping_size); - if (!mapping) { + event = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); + if (!event) { size = 0; goto out; } @@ -200,11 +199,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the digest's algorithm identifier */ if (do_mapping) { - TPM_MEMUNMAP(mapping, mapping_size); + TPM_MEMUNMAP(event, mapping_size); mapping_size = marker - marker_start + halg_size; - mapping = TPM_MEMREMAP((unsigned long)marker_start, - mapping_size); - if (!mapping) { + event = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); + if (!event) { size = 0; goto out; } @@ -220,11 +219,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the digest content itself */ if (do_mapping) { - TPM_MEMUNMAP(mapping, mapping_size); + TPM_MEMUNMAP(event, mapping_size); mapping_size = marker - marker_start; - mapping = TPM_MEMREMAP((unsigned long)marker_start, - mapping_size); - if (!mapping) { + event = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); + if (!event) { size = 0; goto out; } @@ -246,11 +245,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, * we don't need to map it */ if (do_mapping) { - TPM_MEMUNMAP(marker_start, mapping_size); + TPM_MEMUNMAP(event, mapping_size); mapping_size += sizeof(event_field->event_size); - mapping = TPM_MEMREMAP((unsigned long)marker_start, + event = TPM_MEMREMAP((unsigned long)marker_start, mapping_size); - if (!mapping) { + if (!event) { size = 0; goto out; } @@ -260,11 +259,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, + event_field->event_size; size = marker - marker_start; - if ((event->event_type == 0) && (event_field->event_size == 0)) - size = 0; out: if (do_mapping) - TPM_MEMUNMAP(mapping, mapping_size); + TPM_MEMUNMAP(event, mapping_size); return size; }
Re: kexec reboot reset with efi_delete_dummy_variable
On Wed, Apr 17, 2019 at 8:44 PM Dave Young wrote: > So why we still need delete/cleanup the dummy var after entering virt > mode? Am I missing something? I can't think of any reason why you'd need to do this on kexec, but it's vital to do so in the real enter_virtual_mode() path. It should be safe to remove it from kexec_enter_virtual_mode(), but on the other hand it shouldn't be crashing, so I'd recommend figuring out what else is broken first rather than just deleting it.
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
(Cc:ing Peter Jones) On Tue, Apr 9, 2019 at 3:55 PM Claudio Carvalho wrote: > > > On 4/5/19 7:19 PM, Matthew Garrett wrote: > > Based on our experience doing this in UEFI, that's insufficient - you > > want to be able to block individual binaries or leaf certificates > > without dropping trust in an intermediate certificate entirely. > > > We agree that a dbx would be useful for blacklisting particular kernels > signed with given certificate. However, we have been avoiding doing so for > the initial release of secure boot on OpenPOWER. We don't have individual > firmware binaries in OpenPOWER. Kernels are currently the only concern for > the OS secure boot certificates we're discussing here. Also, we have a very > limited keystore space in POWER9. > > Petitboot doesn't have standardized OS kernel verification at all right > now. Having the capability even without dbx seems valuable. I don't see the benefit in attempting to maintain compatibility with existing tooling unless you're going to be *completely* compatible with existing tooling. That means supporting dbx and dbt. > >> The API is still a work in progress. We are planning to publish a document > >> describing the current API and overall design shortly. > > Ok. How are the attributes interpreted by the API? > > > We support a subset of standard EFI variable attributes, and we only use > EFI variables that relate to secure boot. Our goal is not to implement > UEFI. However, we do seek to be compatible with user space tooling and > reuse as much existing infrastructure as possible. We don’t support the > following: EFI_VARIABLE_HARDWARE_ERROR_RECORD, > EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS and > EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS. Ok. I think that's realistically fine. > > > > >> Perhaps the biggest departure is that the secure variables are stored in > >> flash memory that is not lockable. In order to protect the secure > >> variables, hashes of the flash regions where they're stored are written to > >> TPM NVRAM indices. The TPM NVRAM indices we use are write locked at > >> runtime. The sysadmin enqueues update commands in flash. During the next > >> boot, the firmware verifies and processes the commands to update the > >> certificate store and accompanying integrity hashes in the TPM NVRAM > >> indices and write locks them. Before certificates read from flash are > >> used, the certificate store is hashed and compared against the hashes > >> stored from the TPM. The one exception is the PK. We store it in a TPM > >> NVRAM index by itself rather than flash because updates to it must be > >> guaranteed to be atomic. > > What's the behaviour if multiple updates are enqueued? Does reading > > back show a mocked up updated variable or the original state? > > > Our secure variable updates are only applied at boot time. If any one of > them fails, they all fail. So I do the following: 1) Boot 2) Extend the contents of db 3) Extend the contents of db again 4) Read back the contents of db through efivarfs 5) Reboot 6) Read back the contents of db through efivarfs Is what I see in (4) and (6) the same? Does it contain the values form both extensions? > > I'm not really clear on the workflow here. Who's the administrator > > authority? When would they be updating the second level of keys? If > > there's no support for revocation, why would distributions need two > > levels of key in the system database rather than just distributing a > > single intermediate and signing their actual signing certs with that? > > > In OpenPOWER systems, we enable our customers and business partners to > establish and manage the platform key certificate, which is the root of our > key hierarchy. From there, through the KEK, they can delegate authority to > intermediate level organizations, e.g. distros or IT departments or > business operations. Those intermediate level organizations then manage the > code signing certificates in the DB. If this answer doesn’t address your > question, can you please rephrase? Why would the intermediate level organisations not just have entries in db? The main reason we don't do it this way in UEFI is because we need to support dbx, and if you're not supporting dbx I'm not sure I see the benefit.
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
On Fri, Apr 5, 2019 at 2:11 PM Claudio Carvalho wrote: > > > On 4/3/19 7:27 PM, Matthew Garrett wrote: > > Not supporting dbx seems like a pretty significant shortcoming. How > > are signatures meant to be revoked? > > > We began by focusing on certificates for keys that can be added at > runtime. Before adding support for revocation, we plan to gather > additional use cases. In the meantime, unwanted certificates can be > removed by the administrator. Based on our experience doing this in UEFI, that's insufficient - you want to be able to block individual binaries or leaf certificates without dropping trust in an intermediate certificate entirely. > > > > >> PK, KEK and db updates will be signed the same way, that is, using > >> userspace tooling like efitools in PowerNV. As for the authentication > >> descriptors, only the EFI_VARIABLE_AUTHENTICATION_2 descriptor will be > >> supported. > > Is this API documented? > > > The API is still a work in progress. We are planning to publish a document > describing the current API and overall design shortly. Ok. How are the attributes interpreted by the API? > Perhaps the biggest departure is that the secure variables are stored in > flash memory that is not lockable. In order to protect the secure > variables, hashes of the flash regions where they're stored are written to > TPM NVRAM indices. The TPM NVRAM indices we use are write locked at > runtime. The sysadmin enqueues update commands in flash. During the next > boot, the firmware verifies and processes the commands to update the > certificate store and accompanying integrity hashes in the TPM NVRAM > indices and write locks them. Before certificates read from flash are > used, the certificate store is hashed and compared against the hashes > stored from the TPM. The one exception is the PK. We store it in a TPM > NVRAM index by itself rather than flash because updates to it must be > guaranteed to be atomic. What's the behaviour if multiple updates are enqueued? Does reading back show a mocked up updated variable or the original state? > > I think that depends on exactly what problem you're trying to solve. > > Some aspects of the EFI secure boot design end up mirroring the > > economics of the PC ecosystem rather than being inherently good design > > goals, so it'd be helpful to know whether you're taking this solution > > because you want the same three-level key infrastructure or because > > that just leaves you compatible with the tooling. > > > In our use case, the three-level key hierarchy conveniently supports the > concept of (1) an administrator authority, who authorizes (2) other > organizations, e.g., distros, to provide (3) certificates for their code > signing keys. By using efivars, we leverage pre-existing userspace EFI > tools to generate authenticated updates and certificates. Additionally, > pre-existing kernel infrastructure simplifies efivars processing. I'm not really clear on the workflow here. Who's the administrator authority? When would they be updating the second level of keys? If there's no support for revocation, why would distributions need two levels of key in the system database rather than just distributing a single intermediate and signing their actual signing certs with that?
Re: [PATCH] TCG2 log support build fixes for non-x86_64
On Thu, Apr 4, 2019 at 5:41 AM Jarkko Sakkinen wrote: > > On Tue, Apr 02, 2019 at 02:55:54PM -0700, Matthew Garrett wrote: > > Couple of patches to fix ktest reported issues with the crypto-agile log > > format support. > > I guess I squash these to your earlier commits? Works for me.
Re: [PATCH 1/2] efi: Fix cast to pointer from integer of different size in TPM log code
On Thu, Apr 4, 2019 at 5:54 AM Ard Biesheuvel wrote: > > On Wed, 3 Apr 2019 at 04:56, Matthew Garrett > wrote: > > > > 8bfcff4a6a1d9d7226bb63a7da758b82d9ab4373 > > Which tree is this commit in? Sorry, these are in the tpmdd-next tree.
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
On Tue, Apr 2, 2019 at 4:31 PM Claudio Carvalho wrote: > > > On 4/2/19 6:51 PM, Matthew Garrett wrote: > > So you implement the full PK/KEK/db/dbx/dbt infrastructure, and > > updates are signed in the same way? > > For the first version, our firmware will implement a simplistic PK, KEK and > db infrastructure (without dbx and dbt) where only the Setup and User modes > will be supported. Not supporting dbx seems like a pretty significant shortcoming. How are signatures meant to be revoked? > PK, KEK and db updates will be signed the same way, that is, using > userspace tooling like efitools in PowerNV. As for the authentication > descriptors, only the EFI_VARIABLE_AUTHENTICATION_2 descriptor will be > supported. Is this API documented? > > In that case we might be better off with a generic interface for this > > purpose that we can expose on all platforms that implement a secure > > boot key hierarchy. Having an efivarfs that doesn't allow the creation > > of arbitrary attributes may break other existing userland > > expectations. > > > For what it's worth, gsmi uses the efivars infrastructure for EFI-like > variables. My recollection is that at the time the Chromebook firmware still had EFI underpinnings and the gsmi code was largely just an alternate mechanism for calling into something that was fundamentally the EFI variable store. With hindsight I don't think layering this was the right move - we've adjusted the semantics of efivarfs on more than one occasion to deal with the behaviour of real-world EFI platforms, and I don't think it's helpful to end up in a situation where we're trying to keep behaviour consistent among entirely different firmware interfaces. > What might a generic interface look like? It would have to work for > existing secure boot solutions - including EFI - which would seem to imply > changes to userspace tools. I think that depends on exactly what problem you're trying to solve. Some aspects of the EFI secure boot design end up mirroring the economics of the PC ecosystem rather than being inherently good design goals, so it'd be helpful to know whether you're taking this solution because you want the same three-level key infrastructure or because that just leaves you compatible with the tooling.
[PATCH] efi: Include tpm_eventlog.h after asm/efi.h to avoid memcpy breakage
769a8089c1fd2 (x86, efi, kasan: #undef memset/memcpy/memmove per arch) disables the KASAN version of certain memory calls in the EFI boot stub. tpm_eventlog.h references memcpy, so must be included after asm/efi.h in order to allow 769a8089c1fd2 to take effect on its declarations. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/libstub/tpm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index b6e93e14fcf1..777c1e82495a 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -8,9 +8,11 @@ * Thiebaud Weksteen */ #include -#include #include +/* Must be included after asm/efi.h in order to avoid using the wrong memcpy */ +#include + #include "efistub.h" #ifdef CONFIG_RESET_ATTACK_MITIGATION -- 2.21.0.392.gf8f6787159e-goog
Re: [PATCH 1/2] efi: Fix cast to pointer from integer of different size in TPM log code
On Wed, Apr 3, 2019 at 6:41 AM David Laight wrote: > > From: Matthew Garrett > > Sent: 02 April 2019 22:56 > > > > 8bfcff4a6a1d9d7226bb63a7da758b82d9ab4373 introduced a cast from > > efi_physical_address_t to (void *), which are different sizes on 32-bit. > > Fix that. Caught by the 0-day test bot. > > Casting a physical address to 'void *' seems completely wrong. > Also you'd need a guarantee that the address was below 4G or the result > is meaningless. > Looks to me like something is using the wrong types somewhere. We're in UEFI here, not the kernel proper - the firmware functions we call give us back physical addresses, and we're operating with a 1:1 mapping. long is 64 bit on 64 bit systems, and on 32 bit systems we've already asserted that all firmware resources are under 4GB (obviously we're going to have a bad time if they're not, but there's not really anything we can do about that)
[PATCH] TCG2 log support build fixes for non-x86_64
Couple of patches to fix ktest reported issues with the crypto-agile log format support.
[PATCH 1/2] efi: Fix cast to pointer from integer of different size in TPM log code
8bfcff4a6a1d9d7226bb63a7da758b82d9ab4373 introduced a cast from efi_physical_address_t to (void *), which are different sizes on 32-bit. Fix that. Caught by the 0-day test bot. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/libstub/tpm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index b6e93e14fcf1..6b3b507a54eb 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -114,8 +114,8 @@ void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) */ last_entry_size = __calc_tpm2_event_size((void *)last_entry_addr, - (void *)log_location, - false); + (void *)(long)log_location, + false); } else { last_entry_size = sizeof(struct tcpa_event) + ((struct tcpa_event *) last_entry_addr)->event_size; -- 2.21.0.392.gf8f6787159e-goog
[PATCH 2/2] tpm: Fix builds on platforms that lack early_memremap()
On EFI systems, __calc_tpm2_event_size() needs to be able to map tables at early boot time in order to extract information from them. Unfortunately this interacts badly with other architectures that don't provide the early_memremap() interface but which may still have other mechanisms for obtaining crypto-agile logs. Abstract this away so we can avoid the need for two implementations while still avoiding breakage on architectures that don't require remapping of the table. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/tpm.c | 3 +++ include/linux/tpm_eventlog.h | 32 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index f2a13cbb8688..fe48150f06d1 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -4,6 +4,9 @@ * Thiebaud Weksteen */ +#define TPM_MEMREMAP(start, size) early_memremap(start, size) +#define TPM_MEMUNMAP(start, size) early_memunmap(start, size) + #include #include #include diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index d889e12047d9..0ca27bc053af 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -128,6 +128,14 @@ struct tcg_algorithm_info { struct tcg_algorithm_size digest_sizes[]; }; +#ifndef TPM_MEMREMAP +#define TPM_MEMREMAP(start, size) NULL +#endif + +#ifndef TPM_MEMUNMAP +#define TPM_MEMUNMAP(start, size) do{} while(0) +#endif + /** * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry * @event:Pointer to the event whose size should be calculated @@ -171,8 +179,8 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the event header */ if (do_mapping) { mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, -mapping_size); + mapping = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); if (!mapping) { size = 0; goto out; @@ -192,10 +200,10 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the digest's algorithm identifier */ if (do_mapping) { - early_memunmap(mapping, mapping_size); + TPM_MEMUNMAP(mapping, mapping_size); mapping_size = marker - marker_start + halg_size; - mapping = early_memremap((unsigned long)marker_start, -mapping_size); + mapping = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); if (!mapping) { size = 0; goto out; @@ -212,10 +220,10 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, /* Map the digest content itself */ if (do_mapping) { - early_memunmap(mapping, mapping_size); + TPM_MEMUNMAP(mapping, mapping_size); mapping_size = marker - marker_start; - mapping = early_memremap((unsigned long)marker_start, - mapping_size); + mapping = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); if (!mapping) { size = 0; goto out; @@ -238,10 +246,10 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, * we don't need to map it */ if (do_mapping) { - early_memunmap(marker_start, mapping_size); + TPM_MEMUNMAP(marker_start, mapping_size); mapping_size += sizeof(event_field->event_size); - mapping = early_memremap((unsigned long)marker_start, -mapping_size); + mapping = TPM_MEMREMAP((unsigned long)marker_start, + mapping_size); if (!mapping) { size = 0; goto out; @@ -256,7 +264,7 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, size = 0; out: if (do_mapping) - early_memunmap(mapping, mapping_size); + TPM_MEMUNMAP(mapping, mapping_size); return size; } -- 2.21.0.392.gf8f6787159e-goog
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
On Tue, Apr 2, 2019 at 2:11 PM Claudio Carvalho wrote: > We want to use the efivarfs for compatibility with existing userspace > tools. We will track and match any EFI changes that affect us. So you implement the full PK/KEK/db/dbx/dbt infrastructure, and updates are signed in the same way? > Our use case is restricted to secure boot - this is not going to be a > general purpose EFI variable implementation. In that case we might be better off with a generic interface for this purpose that we can expose on all platforms that implement a secure boot key hierarchy. Having an efivarfs that doesn't allow the creation of arbitrary attributes may break other existing userland expectations.
Re: [PATCH 0/4] Enabling secure boot on PowerNV systems
On Tue, Apr 2, 2019 at 11:15 AM Claudio Carvalho wrote: > 1. Enable efivarfs by selecting CONFIG_EFI in the CONFIG_OPAL_SECVAR >introduced in this patch set. With CONFIG_EFIVAR_FS, userspace tools can >be used to manage the secure variables. efivarfs has some pretty significant behavioural semantics that directly reflect the EFI specification. Using it to expose non-EFI variable data feels like it's going to increase fragility - there's a risk that we'll change things in a way that makes sense for the EFI spec but breaks your use case. Is the desire to use efivarfs to maintain consistency with existing userland tooling, or just to avoid having a separate filesystem?
Re: Add support for TCG2 log format on UEFI systems
On Tue, Apr 2, 2019 at 6:07 AM Jarkko Sakkinen wrote: > Reviewed-by: Jarkko Sakkinen > Tested-by: Jarkko Sakkinen > > I'll apply all patches soonish and include them to the next PR. Thanks! Looks like I need some fixes to deal with non-x86 architectures, I'll get on that today.
Re: Add support for TCG2 log format on UEFI systems
On Mon, Apr 1, 2019 at 4:52 PM Jarkko Sakkinen wrote: > > On Wed, Feb 27, 2019 at 12:26:54PM -0800, Matthew Garrett wrote: > > Identical to V4, but based on tpmdd-next > > OK, so on my GLK NUC I get valid final log and invalid event log > after adding some extra klogs. > > I.e. > > - if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) > + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { Just to make sure - are you booting via the EFI boot stub? We need to obtain the boot log before ExitBootServices() is called, so if you're booting directly into the 32-bit entry point (eg, by using the "linux" command in grub) then you won't get a log.
Re: Add support for TCG2 log format on UEFI systems
On Thu, Mar 14, 2019 at 2:35 AM Jarkko Sakkinen wrote: > > On Wed, Feb 27, 2019 at 12:26:54PM -0800, Matthew Garrett wrote: > > Identical to V4, but based on tpmdd-next > > This is not found /sys/kernel/security/tpm0/ascii_bios_measurements That's expected - the existing kernel TCG2 log code doesn't expose ascii_bios_measurements, only binary_bios_measurements. This patchset doesn't change that.
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Mon, Mar 11, 2019 at 9:55 AM Mimi Zohar wrote: > > On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote: > > Hm. And this only happens on certain firmware versions? If something's > > stepping on boot_params then we have bigger problems. > > I was seeing this problem before and after updating the system > firmware on my laptop last summer. If updating the firmware had > resolved the problem, I wouldn't have included this patch. Ah, sorry, when you said that you saw this with older firmware I thought that meant that newer firmware fixed it. What machine are you testing on?
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Fri, Mar 8, 2019 at 10:43 AM Mimi Zohar wrote: > FYI, efi_printk() works before exit_boot(), but not afterwards. The > system hangs. efi_printk() uses boot services to print, so that's not unexpected :) It would probably be sensible to return an error rather than crash, though…
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar wrote: > > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote: > > Is the issue that it gives incorrect results on the first read, or is > > the issue that it gives incorrect results before ExitBootServices() is > > called? If the former then we should read twice in the boot stub, if > > the latter then we should figure out a way to do this immediately > > after ExitBootServices() instead. > > Detecting the secure boot mode isn't the problem. On boot, I am > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits > "Secure boot could not be determined". > > In efi_main() the secure_boot mode is initially unset, so > efi_get_secureboot() is called. efi_get_secureboot() returns the > secure_boot mode correctly as enabled. The problem seems to be in > saving the secure_boot mode for later use. Hm. And this only happens on certain firmware versions? If something's stepping on boot_params then we have bigger problems.
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Thu, Mar 7, 2019 at 2:48 PM Mimi Zohar wrote: > I added this last attempt because I'm seeing this on my laptop, with > some older, buggy firmware. Is the issue that it gives incorrect results on the first read, or is the issue that it gives incorrect results before ExitBootServices() is called? If the former then we should read twice in the boot stub, if the latter then we should figure out a way to do this immediately after ExitBootServices() instead.
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Thu, Mar 7, 2019 at 2:38 PM Justin Forbes wrote: > On Thu, Mar 7, 2019 at 4:29 PM Matthew Garrett wrote: >> >> On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar wrote: >> > >> > The secure boot mode may not be detected on boot for some reason (eg. >> > buggy firmware). This patch attempts one more time to detect the >> > secure boot mode. >> >> Do we have cases where this has actually been seen? I'm not sure what >> the circumstances are that would result in this behaviour. > > > We have never seen it in practice, though we only ever do anything with it > with x86, so it is possible that some other platforms maybe? I'm not sure that it buys us anything to check this in both the boot stub and the running kernel. If a platform *is* giving us different results, anything else relying on the information from the boot stub is also going to be broken, so we should do this centrally rather than in the IMA code.
Re: [PATCH 3/3] x86/ima: retry detecting secure boot mode
On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar wrote: > > The secure boot mode may not be detected on boot for some reason (eg. > buggy firmware). This patch attempts one more time to detect the > secure boot mode. Do we have cases where this has actually been seen? I'm not sure what the circumstances are that would result in this behaviour.
[PATCH V5 1/4] tpm: Abstract crypto agile event size calculations
From: Matthew Garrett We need to calculate the size of crypto agile events in multiple locations, including in the EFI boot stub. The easiest way to do this is to put it in a header file as an inline and leave a wrapper to ensure we don't end up with multiple copies of it embedded in the existing code. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 47 +- include/linux/tpm_eventlog.h | 68 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index f824563fc28d..1a977bdd3bd2 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,52 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - struct tcg_efi_specid_event_head *efispecid; - struct tcg_event_field *event_field; - void *marker; - void *marker_start; - u32 halg_size; - size_t size; - u16 halg; - int i; - int j; - - marker = event; - marker_start = marker; - marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) - + sizeof(event->count); - - efispecid = (struct tcg_efi_specid_event_head *)event_header->event; - - /* Check if event is malformed. */ - if (event->count > efispecid->num_algs) - return 0; - - for (i = 0; i < event->count; i++) { - halg_size = sizeof(event->digests[i].alg_id); - memcpy(&halg, marker, halg_size); - marker = marker + halg_size; - for (j = 0; j < efispecid->num_algs; j++) { - if (halg == efispecid->digest_sizes[j].alg_id) { - marker += - efispecid->digest_sizes[j].digest_size; - break; - } - } - /* Algorithm without known length. Such event is unparseable. */ - if (j == efispecid->num_algs) - return 0; - } - - event_field = (struct tcg_event_field *)marker; - marker = marker + sizeof(event_field->event_size) - + event_field->event_size; - size = marker - marker_start; - - if ((event->event_type == 0) && (event_field->event_size == 0)) - return 0; - - return size; + return __calc_tpm2_event_size(event, event_header); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index 81519f163211..6a86144e13f1 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -112,4 +112,72 @@ struct tcg_pcr_event2_head { struct tpm_digest digests[]; } __packed; +/** + * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry + * @event:Pointer to the event whose size should be calculated + * @event_header: Pointer to the initial event containing the digest lengths + * + * The TPM2 event log format can contain multiple digests corresponding to + * separate PCR banks, and also contains a variable length of the data that + * was measured. This requires knowledge of how long each digest type is, + * and this information is contained within the first event in the log. + * + * We calculate the length by examining the number of events, and then looking + * at each event in turn to determine how much space is used for events in + * total. Once we've done this we know the offset of the data length field, + * and can calculate the total size of the event. + * + * Return: size of the event on success, <0 on failure + */ + +static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, +struct tcg_pcr_event *event_header) +{ + struct tcg_efi_specid_event_head *efispecid; + struct tcg_event_field *event_field; + void *marker; + void *marker_start; + u32 halg_size; + size_t size; + u16 halg; + int i; + int j; + + marker = event; + marker_start = marker; + marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) + + sizeof(event->count); + + efispecid = (struct tcg_efi_specid_event_head *)event_header->event; + + /* Check if event is malformed. */ + if (event->count > efispecid->num_algs) + return 0; + + for (i = 0; i < event->count; i++) { + halg_size = sizeof(event->digests[i].alg_id); + memcpy(&halg, marker, halg_size); + marker = marker + halg_size; + for (j = 0; j < efispecid->num_al
[PATCH V5 3/4] tpm: Append the final event log to the TPM event log
From: Matthew Garrett Any events that are logged after GetEventsLog() is called are logged to the EFI Final Events table. These events are defined as being in the crypto agile log format, so we can just append them directly to the existing log if it's in the same format. In theory we can also construct old-style SHA1 log entries for devices that only return logs in that format, but EDK2 doesn't generate the final event log in that case so it doesn't seem worth it at the moment. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/efi.c | 50 - 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 3e673ab22cb4..9179cf6bdee9 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -21,10 +21,13 @@ int tpm_read_log_efi(struct tpm_chip *chip) { + struct efi_tcg2_final_events_table *final_tbl = NULL; struct linux_efi_tpm_eventlog *log_tbl; struct tpm_bios_log *log; u32 log_size; u8 tpm_log_version; + void *tmp; + int ret; if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) return -ENODEV; @@ -52,15 +55,48 @@ int tpm_read_log_efi(struct tpm_chip *chip) /* malloc EventLog space */ log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); - if (!log->bios_event_log) - goto err_memunmap; - log->bios_event_log_end = log->bios_event_log + log_size; + if (!log->bios_event_log) { + ret = -ENOMEM; + goto out; + } + log->bios_event_log_end = log->bios_event_log + log_size; tpm_log_version = log_tbl->version; - memunmap(log_tbl); - return tpm_log_version; -err_memunmap: + ret = tpm_log_version; + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR || + efi_tpm_final_log_size == 0 || + tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) + goto out; + + final_tbl = memremap(efi.tpm_final_log, +sizeof(*final_tbl) + efi_tpm_final_log_size, +MEMREMAP_WB); + if (!final_tbl) { + pr_err("Could not map UEFI TPM final log\n"); + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + tmp = krealloc(log->bios_event_log, + log_size + efi_tpm_final_log_size, + GFP_KERNEL); + if (!tmp) { + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + log->bios_event_log = tmp; + memcpy((void *)log->bios_event_log + log_size, + final_tbl->events, efi_tpm_final_log_size); + log->bios_event_log_end = log->bios_event_log + + log_size + efi_tpm_final_log_size; + +out: + memunmap(final_tbl); memunmap(log_tbl); - return -ENOMEM; + return ret; } -- 2.21.0.352.gf09ad66450-goog
[PATCH V5 4/4] efi: Attempt to get the TCG2 event log in the boot stub
From: Matthew Garrett Right now we only attempt to obtain the SHA1-only event log. The protocol also supports a crypto agile log format, which contains digests for all algorithms in use. Attempt to obtain this first, and fall back to obtaining the older format if the system doesn't support it. This is lightly complicated by the event sizes being variable (as we don't know in advance which algorithms are in use), and the interface giving us back a pointer to the start of the final entry rather than a pointer to the end of the log - as a result, we need to parse the final entry to figure out its length in order to know how much data to copy up to the OS. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/libstub/tpm.c | 50 -- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index a90b0b8fc69a..523cd07c551c 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -59,7 +59,7 @@ void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) #endif -static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) +void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) { efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID; efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; @@ -69,6 +69,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) unsigned long first_entry_addr, last_entry_addr; size_t log_size, last_entry_size; efi_bool_t truncated; + int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; void *tcg2_protocol = NULL; status = efi_call_early(locate_protocol, &tcg2_guid, NULL, @@ -76,14 +77,20 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) if (status != EFI_SUCCESS) return; - status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol, - EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2, - &log_location, &log_last_entry, &truncated); - if (status != EFI_SUCCESS) - return; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + + if (status != EFI_SUCCESS || !log_location) { + version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + if (status != EFI_SUCCESS || !log_location) + return; + + } - if (!log_location) - return; first_entry_addr = (unsigned long) log_location; /* @@ -98,8 +105,23 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) * We need to calculate its size to deduce the full size of * the logs. */ - last_entry_size = sizeof(struct tcpa_event) + - ((struct tcpa_event *) last_entry_addr)->event_size; + if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { + /* +* The TCG2 log format has variable length entries, +* and the information to decode the hash algorithms +* back into a size is contained in the first entry - +* pass a pointer to the final entry (to calculate its +* size) and the first entry (so we know how long each +* digest is) +*/ + last_entry_size = + __calc_tpm2_event_size((void *)last_entry_addr, + (void *)log_location, + false); + } else { + last_entry_size = sizeof(struct tcpa_event) + + ((struct tcpa_event *) last_entry_addr)->event_size; + } log_size = log_last_entry - log_location + last_entry_size; } @@ -116,7 +138,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) memset(log_tbl, 0, sizeof(*log_tbl) + log_size); log_tbl->size = log_size; - log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + log_tbl->version = version; memcpy(log_tbl->log, (void *) first_entry_addr, log_size); status = efi_call_early(install_configuration_table, @@ -128,9 +150,3 @@ static void efi_ret
[PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett UEFI systems provide a boot services protocol for obtaining the TPM event log, but this is unusable after ExitBootServices() is called. Unfortunately ExitBootServices() itself triggers additional TPM events that then can't be obtained using this protocol. The platform provides a mechanism for the OS to obtain these events by recording them to a separate UEFI configuration table which the OS can then map. Unfortunately this table isn't self describing in terms of providing its length, so we need to parse the events inside it to figure out how long it is. Since the table isn't mapped at this point, we need to extend the length calculation function to be able to map the event as it goes along. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 2 +- drivers/firmware/efi/efi.c | 2 + drivers/firmware/efi/tpm.c | 51 - include/linux/efi.h | 9 +++ include/linux/tpm_eventlog.h | 94 +--- 5 files changed, 148 insertions(+), 10 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index 1a977bdd3bd2..de1d9f7e5a92 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,7 +40,7 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - return __calc_tpm2_event_size(event, event_header); + return __calc_tpm2_event_size(event, event_header, false); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 4c46ff6f2242..bf4e9a254e23 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -53,6 +53,7 @@ struct efi __read_mostly efi = { .mem_attr_table = EFI_INVALID_TABLE_ADDR, .rng_seed = EFI_INVALID_TABLE_ADDR, .tpm_log= EFI_INVALID_TABLE_ADDR, + .tpm_final_log = EFI_INVALID_TABLE_ADDR, .mem_reserve= EFI_INVALID_TABLE_ADDR, }; EXPORT_SYMBOL(efi); @@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] = { {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table}, {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed}, {LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log}, + {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", &efi.tpm_final_log}, {LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve}, {NULL_GUID, NULL, NULL}, }; diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 0cbeb3d46b18..2ccaa6661aaf 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -10,24 +10,50 @@ #include #include #include +#include #include +int efi_tpm_final_log_size; +EXPORT_SYMBOL(efi_tpm_final_log_size); + +static int tpm2_calc_event_log_size(void *data, int count, void *size_info) +{ + struct tcg_pcr_event2_head *header; + int event_size, size = 0; + + while (count > 0) { + header = data + size; + event_size = __calc_tpm2_event_size(header, size_info, true); + if (event_size == 0) + return -1; + size += event_size; + } + + return size; +} + /* * Reserve the memory associated with the TPM Event Log configuration table. */ int __init efi_tpm_eventlog_init(void) { struct linux_efi_tpm_eventlog *log_tbl; + struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; - if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { + /* +* We can't calculate the size of the final events without the +* first entry in the TPM log, so bail here. +*/ return 0; + } log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl)); if (!log_tbl) { pr_err("Failed to map TPM Event Log table @ 0x%lx\n", - efi.tpm_log); + efi.tpm_log); efi.tpm_log = EFI_INVALID_TABLE_ADDR; return -ENOMEM; } @@ -35,6 +61,27 @@ int __init efi_tpm_eventlog_init(void) tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); early_memunmap(log_tbl, sizeof(*log_tbl)); + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) + return 0; + + final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); + + if (!final_tbl) { + pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n", + efi.tpm_final_log); +
Add support for TCG2 log format on UEFI systems
Identical to V4, but based on tpmdd-next
Re: [PATCH V4 2/4] tpm: Reserve the TPM final events table
On Wed, Feb 27, 2019 at 6:03 AM Jarkko Sakkinen wrote: > My guess is that your patches are based a later 5.0-rcX. Unfortunately I > cannot update my master at this point because my 5.1 PR was taken to > security tree and rebasing would change the commit IDs of 5.1 content > because security/next-general does not yet contain those patches. Bother. I'll rebase against your tree.
[PATCH V4 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett UEFI systems provide a boot services protocol for obtaining the TPM event log, but this is unusable after ExitBootServices() is called. Unfortunately ExitBootServices() itself triggers additional TPM events that then can't be obtained using this protocol. The platform provides a mechanism for the OS to obtain these events by recording them to a separate UEFI configuration table which the OS can then map. Unfortunately this table isn't self describing in terms of providing its length, so we need to parse the events inside it to figure out how long it is. Since the table isn't mapped at this point, we need to extend the length calculation function to be able to map the event as it goes along. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 2 +- drivers/firmware/efi/efi.c | 2 + drivers/firmware/efi/tpm.c | 51 - include/linux/efi.h | 9 +++ include/linux/tpm_eventlog.h | 94 +--- 5 files changed, 148 insertions(+), 10 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index dc12e1cbd03a..2cfdf1db4363 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,7 +40,7 @@ static int calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - return __calc_tpm2_event_size(event, event_header); + return __calc_tpm2_event_size(event, event_header, false); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 55b77c576c42..6b11c41e0575 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -53,6 +53,7 @@ struct efi __read_mostly efi = { .mem_attr_table = EFI_INVALID_TABLE_ADDR, .rng_seed = EFI_INVALID_TABLE_ADDR, .tpm_log= EFI_INVALID_TABLE_ADDR, + .tpm_final_log = EFI_INVALID_TABLE_ADDR, .mem_reserve= EFI_INVALID_TABLE_ADDR, }; EXPORT_SYMBOL(efi); @@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] = { {EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", &efi.mem_attr_table}, {LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", &efi.rng_seed}, {LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", &efi.tpm_log}, + {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", &efi.tpm_final_log}, {LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", &efi.mem_reserve}, {NULL_GUID, NULL, NULL}, }; diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c index 0cbeb3d46b18..2ccaa6661aaf 100644 --- a/drivers/firmware/efi/tpm.c +++ b/drivers/firmware/efi/tpm.c @@ -10,24 +10,50 @@ #include #include #include +#include #include +int efi_tpm_final_log_size; +EXPORT_SYMBOL(efi_tpm_final_log_size); + +static int tpm2_calc_event_log_size(void *data, int count, void *size_info) +{ + struct tcg_pcr_event2_head *header; + int event_size, size = 0; + + while (count > 0) { + header = data + size; + event_size = __calc_tpm2_event_size(header, size_info, true); + if (event_size == 0) + return -1; + size += event_size; + } + + return size; +} + /* * Reserve the memory associated with the TPM Event Log configuration table. */ int __init efi_tpm_eventlog_init(void) { struct linux_efi_tpm_eventlog *log_tbl; + struct efi_tcg2_final_events_table *final_tbl; unsigned int tbl_size; - if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) { + /* +* We can't calculate the size of the final events without the +* first entry in the TPM log, so bail here. +*/ return 0; + } log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl)); if (!log_tbl) { pr_err("Failed to map TPM Event Log table @ 0x%lx\n", - efi.tpm_log); + efi.tpm_log); efi.tpm_log = EFI_INVALID_TABLE_ADDR; return -ENOMEM; } @@ -35,6 +61,27 @@ int __init efi_tpm_eventlog_init(void) tbl_size = sizeof(*log_tbl) + log_tbl->size; memblock_reserve(efi.tpm_log, tbl_size); early_memunmap(log_tbl, sizeof(*log_tbl)); + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) + return 0; + + final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl)); + + if (!final_tbl) { + pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n", + efi.tpm_final_log); +
[PATCH V4 0/4] Add support for TCG2 event logs on EFI systems
This patchset adds support for obtaining the TCG2 format event log on EFI systems, along with support for copying up the final event log to capture events that occur after the primary log is obtained. V4 is identical to previous versions, except for tpm_read_log_efi() in patch 3 being reworked to reduce nesting.
[PATCH V4 4/4] efi: Attempt to get the TCG2 event log in the boot stub
From: Matthew Garrett Right now we only attempt to obtain the SHA1-only event log. The protocol also supports a crypto agile log format, which contains digests for all algorithms in use. Attempt to obtain this first, and fall back to obtaining the older format if the system doesn't support it. This is lightly complicated by the event sizes being variable (as we don't know in advance which algorithms are in use), and the interface giving us back a pointer to the start of the final entry rather than a pointer to the end of the log - as a result, we need to parse the final entry to figure out its length in order to know how much data to copy up to the OS. Signed-off-by: Matthew Garrett --- drivers/firmware/efi/libstub/tpm.c | 50 -- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c index a90b0b8fc69a..523cd07c551c 100644 --- a/drivers/firmware/efi/libstub/tpm.c +++ b/drivers/firmware/efi/libstub/tpm.c @@ -59,7 +59,7 @@ void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg) #endif -static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) +void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg) { efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID; efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID; @@ -69,6 +69,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) unsigned long first_entry_addr, last_entry_addr; size_t log_size, last_entry_size; efi_bool_t truncated; + int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; void *tcg2_protocol = NULL; status = efi_call_early(locate_protocol, &tcg2_guid, NULL, @@ -76,14 +77,20 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) if (status != EFI_SUCCESS) return; - status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol, - EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2, - &log_location, &log_last_entry, &truncated); - if (status != EFI_SUCCESS) - return; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + + if (status != EFI_SUCCESS || !log_location) { + version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + status = efi_call_proto(efi_tcg2_protocol, get_event_log, + tcg2_protocol, version, &log_location, + &log_last_entry, &truncated); + if (status != EFI_SUCCESS || !log_location) + return; + + } - if (!log_location) - return; first_entry_addr = (unsigned long) log_location; /* @@ -98,8 +105,23 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) * We need to calculate its size to deduce the full size of * the logs. */ - last_entry_size = sizeof(struct tcpa_event) + - ((struct tcpa_event *) last_entry_addr)->event_size; + if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) { + /* +* The TCG2 log format has variable length entries, +* and the information to decode the hash algorithms +* back into a size is contained in the first entry - +* pass a pointer to the final entry (to calculate its +* size) and the first entry (so we know how long each +* digest is) +*/ + last_entry_size = + __calc_tpm2_event_size((void *)last_entry_addr, + (void *)log_location, + false); + } else { + last_entry_size = sizeof(struct tcpa_event) + + ((struct tcpa_event *) last_entry_addr)->event_size; + } log_size = log_last_entry - log_location + last_entry_size; } @@ -116,7 +138,7 @@ static void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) memset(log_tbl, 0, sizeof(*log_tbl) + log_size); log_tbl->size = log_size; - log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + log_tbl->version = version; memcpy(log_tbl->log, (void *) first_entry_addr, log_size); status = efi_call_early(install_configuration_table, @@ -128,9 +150,3 @@ static void efi_ret
[PATCH V4 3/4] tpm: Append the final event log to the TPM event log
From: Matthew Garrett Any events that are logged after GetEventsLog() is called are logged to the EFI Final Events table. These events are defined as being in the crypto agile log format, so we can just append them directly to the existing log if it's in the same format. In theory we can also construct old-style SHA1 log entries for devices that only return logs in that format, but EDK2 doesn't generate the final event log in that case so it doesn't seem worth it at the moment. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/efi.c | 50 - 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index 3e673ab22cb4..9179cf6bdee9 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -21,10 +21,13 @@ int tpm_read_log_efi(struct tpm_chip *chip) { + struct efi_tcg2_final_events_table *final_tbl = NULL; struct linux_efi_tpm_eventlog *log_tbl; struct tpm_bios_log *log; u32 log_size; u8 tpm_log_version; + void *tmp; + int ret; if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) return -ENODEV; @@ -52,15 +55,48 @@ int tpm_read_log_efi(struct tpm_chip *chip) /* malloc EventLog space */ log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); - if (!log->bios_event_log) - goto err_memunmap; - log->bios_event_log_end = log->bios_event_log + log_size; + if (!log->bios_event_log) { + ret = -ENOMEM; + goto out; + } + log->bios_event_log_end = log->bios_event_log + log_size; tpm_log_version = log_tbl->version; - memunmap(log_tbl); - return tpm_log_version; -err_memunmap: + ret = tpm_log_version; + + if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR || + efi_tpm_final_log_size == 0 || + tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) + goto out; + + final_tbl = memremap(efi.tpm_final_log, +sizeof(*final_tbl) + efi_tpm_final_log_size, +MEMREMAP_WB); + if (!final_tbl) { + pr_err("Could not map UEFI TPM final log\n"); + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + tmp = krealloc(log->bios_event_log, + log_size + efi_tpm_final_log_size, + GFP_KERNEL); + if (!tmp) { + kfree(log->bios_event_log); + ret = -ENOMEM; + goto out; + } + + log->bios_event_log = tmp; + memcpy((void *)log->bios_event_log + log_size, + final_tbl->events, efi_tpm_final_log_size); + log->bios_event_log_end = log->bios_event_log + + log_size + efi_tpm_final_log_size; + +out: + memunmap(final_tbl); memunmap(log_tbl); - return -ENOMEM; + return ret; } -- 2.21.0.rc0.258.g878e2cd30e-goog
[PATCH V4 1/4] tpm: Abstract crypto agile event size calculations
From: Matthew Garrett We need to calculate the size of crypto agile events in multiple locations, including in the EFI boot stub. The easiest way to do this is to put it in a header file as an inline and leave a wrapper to ensure we don't end up with multiple copies of it embedded in the existing code. Signed-off-by: Matthew Garrett --- drivers/char/tpm/eventlog/tpm2.c | 47 +- include/linux/tpm_eventlog.h | 68 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index d8b77133a83a..dc12e1cbd03a 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -40,52 +40,7 @@ static int calc_tpm2_event_size(struct tcg_pcr_event2_head *event, struct tcg_pcr_event *event_header) { - struct tcg_efi_specid_event_head *efispecid; - struct tcg_event_field *event_field; - void *marker; - void *marker_start; - u32 halg_size; - size_t size; - u16 halg; - int i; - int j; - - marker = event; - marker_start = marker; - marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) - + sizeof(event->count); - - efispecid = (struct tcg_efi_specid_event_head *)event_header->event; - - /* Check if event is malformed. */ - if (event->count > efispecid->num_algs) - return 0; - - for (i = 0; i < event->count; i++) { - halg_size = sizeof(event->digests[i].alg_id); - memcpy(&halg, marker, halg_size); - marker = marker + halg_size; - for (j = 0; j < efispecid->num_algs; j++) { - if (halg == efispecid->digest_sizes[j].alg_id) { - marker += - efispecid->digest_sizes[j].digest_size; - break; - } - } - /* Algorithm without known length. Such event is unparseable. */ - if (j == efispecid->num_algs) - return 0; - } - - event_field = (struct tcg_event_field *)marker; - marker = marker + sizeof(event_field->event_size) - + event_field->event_size; - size = marker - marker_start; - - if ((event->event_type == 0) && (event_field->event_size == 0)) - return 0; - - return size; + return __calc_tpm2_event_size(event, event_header); } static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h index f47342361e87..09c19d506b69 100644 --- a/include/linux/tpm_eventlog.h +++ b/include/linux/tpm_eventlog.h @@ -117,4 +117,72 @@ struct tcg_pcr_event2_head { struct tpm2_digest digests[]; } __packed; +/** + * __calc_tpm2_event_size - calculate the size of a TPM2 event log entry + * @event:Pointer to the event whose size should be calculated + * @event_header: Pointer to the initial event containing the digest lengths + * + * The TPM2 event log format can contain multiple digests corresponding to + * separate PCR banks, and also contains a variable length of the data that + * was measured. This requires knowledge of how long each digest type is, + * and this information is contained within the first event in the log. + * + * We calculate the length by examining the number of events, and then looking + * at each event in turn to determine how much space is used for events in + * total. Once we've done this we know the offset of the data length field, + * and can calculate the total size of the event. + * + * Return: size of the event on success, <0 on failure + */ + +static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event, +struct tcg_pcr_event *event_header) +{ + struct tcg_efi_specid_event_head *efispecid; + struct tcg_event_field *event_field; + void *marker; + void *marker_start; + u32 halg_size; + size_t size; + u16 halg; + int i; + int j; + + marker = event; + marker_start = marker; + marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type) + + sizeof(event->count); + + efispecid = (struct tcg_efi_specid_event_head *)event_header->event; + + /* Check if event is malformed. */ + if (event->count > efispecid->num_algs) + return 0; + + for (i = 0; i < event->count; i++) { + halg_size = sizeof(event->digests[i].alg_id); + memcpy(&halg, marker, halg_size); + marker = marker + halg_size; + for (j = 0; j < efispecid->num_al
Re: [GIT PULL] Kernel lockdown for secure boot
On Thu, Apr 5, 2018 at 10:59 AM Alan Cox wrote: > VT-D Once Intel provide that on all hardware and actually make it work reliably with their graphics chipsets it's certainly a solution for the PCI DMA problem, but right now it's still effectively undeployable for a lot of real world cases. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
On Wed, Apr 4, 2018 at 4:25 PM James Morris wrote: > It's surely reasonable to allow an already secure-booted system to be > debugged without needing to be rebooted. alt-sysrq-x from a physical console will do that. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 5:05 PM Peter Dolding wrote: > > If you don't have secure boot then an attacker with root can modify your > > bootloader or kernel, and on next boot lockdown can be silently disabled. > Stop being narrow minded you don't need secure boot to protect > bootloader or kernel the classic is only boot from read only media. And if you use another protected path you can set the appropriate bootparam flag or pass the appropriate kernel command line argument and gain the same functionality. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 1:01 PM Thomas Gleixner wrote: > Now where the disagreement lies is the way how the uid/ring0 aspect is tied > to secure boot, which makes it impossible to be useful independent of > Secure Boot. It doesn't - you can pass a command line parameter that enables it, or your bootloader can set the bootparams flag. I don't see a fundamental problem with offering the opportunity to change it at runtime, other than that some stuff that was previously initialised may have to be torn down. The reason for having the UEFI boot stub *optionally* check the secure boot state itself and make a policy decision (rather than having the signed bootloader do so) is because the kernel can be launched directly by the firmware. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 9:39 AM Andy Lutomirski wrote: > On Wed, Apr 4, 2018 at 9:22 AM, Matthew Garrett wrote: > > If you don't have secure boot then an attacker with root can modify your > > bootloader or kernel, and on next boot lockdown can be silently disabled. > This has been rebutted over and over and over. Secure boot is not the > only verified boot mechanism in the world. Other, better, much more > auditable, and much simpler mechanisms have been around for a long, > long time. Right and if you *know* that you're in that situation then you either turn it on in bootparams from the verified bootloader (which we can't do in UEFI because the *firmware* can be the bootloader thanks to the EFI boot stub) or you enable it from userland later (I can't remember if this version of the patchset provides that functionality, but a previous one did). > > Which is why Shim allows you to disable validation if you prove physical > > user presence. > And that's a giant hack. The actual feature should be that a user > proves physical presence and thus disables lockdown *without* > disabling verification. That's a completely reasonable feature request. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 11:56 PM Peter Dolding wrote: > On Wed, Apr 4, 2018 at 11:13 AM, Matthew Garrett wrote: > > There are four cases: > > > > Verified Boot off, lockdown off: Status quo in distro and mainline kernels > > Verified Boot off, lockdown on: Perception of security improvement that's > > trivially circumvented (and so bad) > > Verified Boot on, lockdown off: Perception of security improvement that's > > trivially circumvented (and so bad), status quo in mainline kernels > > Verified Boot on, lockdown on: Security improvement, status quo in distro > > kernels > > > > Of these four options, only two make sense. The most common implementation > > of Verified Boot on x86 platforms is UEFI Secure Boot, > Stop right there. Verified boot does not have to be UEFI secureboot. >You could be using a uboot verified boot or > https://www.coreboot.org/git-docs/Intel/vboot.html google vboot. > Neither of these provide flags to kernel to say they have been > performed. They can be modified to set the appropriate bit in the bootparams - the reason we can't do that in the UEFI case is that Linux can be built as a UEFI binary that the firmware execute directly, and so the firmware has no way to set that flag. > Now Verified Boot on, lockdown off. Insanely this can be required in > diagnostic on some embedded platform because EFI secureboot does not > have a off switch.These are platforms where they don't boot if > they don't have a PK and KEK set installed. Yes some of these is jtag > the PK and KEK set in. > The fact that this Verified Boot on, lockdown off causes trouble > points to a clear problem. User owns the hardware they should have > the right to defeat secureboot if they wish to. Which is why Shim allows you to disable validation if you prove physical user presence. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 6:52 AM Theodore Y. Ts'o wrote: > On Wed, Apr 04, 2018 at 02:33:37PM +0100, David Howells wrote: > > Theodore Y. Ts'o wrote: > > > > > Whoa. Why doesn't lockdown prevent kexec? Put another away, why > > > isn't this a problem for people who are fearful that Linux could be > > > used as part of a Windows boot virus in a Secure UEFI context? > > > > Lockdown mode restricts kexec to booting an authorised image (where the > > authorisation may be by signature or by IMA). > If that's true, then Matthew's assertion that lockdown w/o secure boot > is insecure goes away, no? If you don't have secure boot then an attacker with root can modify your bootloader or kernel, and on next boot lockdown can be silently disabled. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 5:57 AM Theodore Y. Ts'o wrote: > On Wed, Apr 04, 2018 at 04:30:18AM +, Matthew Garrett wrote: > > What I'm afraid of is this turning into a "security" feature that ends up > > being circumvented in most scenarios where it's currently deployed - eg, > > module signatures are mostly worthless in the non-lockdown case because you > > can just grab the sig_enforce symbol address and then kexec a preamble that > > flips it back to N regardless of the kernel config. > Whoa. Why doesn't lockdown prevent kexec? Put another away, why > isn't this a problem for people who are fearful that Linux could be > used as part of a Windows boot virus in a Secure UEFI context? It does - I was talking about the non-lockdown case. In the lockdown case you can only kexec images you trust, so there's no problem. Red Hat have been shipping a signed kdump image for years. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Wed, Apr 4, 2018 at 9:09 AM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 9:30 PM, Matthew Garrett wrote: > > > > Bear in mind that I'm talking about defaults here > Mattyhew, I really want you to look yourself in the mirror. > Those defaults are really horrible defautls for real technical reasons. > You asked me why when I questioned this, but then when I replied, you > entirely ignored it. > So let me repeat: the defaults are *horrible*. They are horrible for a > very simple reason: kernel behavior changes that depend on some subtle > boot difference are truly nasty to debug, and nasty to get coverage > for. They're the defaults that the mainline distros have been shipping for years. So what are you actually asking for here? If you're saying that it should be possible to enable the lockdown functionality even in the absence of any kind of verified boot, then yes, I agree - I just think it makes a poor distro default to have that be the case out of the box. If you're saying that it should be possible to disable the lockdown functionality even in the presence of any kind of verified boot, then yes, I agree - I just think it makes a poor distro default to have that be the case out of the box. You're arguing against a patch that provides the default policy that distros want to ship. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 7:34 PM Alexei Starovoitov < alexei.starovoi...@gmail.com> wrote: > If the only thing that folks are paranoid about is reading > arbitrary kernel memory with bpf_probe_read() helper > then preferred patch would be to disable it during verification > when in lockdown mode. > No run-time overhead and android folks will be happy > that lockdown doesn't break their work. > They converted out-of-tree networking accounting > module and corresponding user daemon to use bpf: https://www.linuxplumbersconf.org/2017/ocw/system/presentations/4791/original/eBPF%20cgroup%20filters%20for%20data%20usage%20accounting%20on%20Android.pdf An alternative would be to only disable kernel reads if the kernel contains secrets that aren't supposed to be readable by root. If the keyring is configured such that root can read everything, it seems like less of a concern? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 6:43 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 6:13 PM, Matthew Garrett wrote: > > > > There are four cases: > No. > Matthew., stop with the agenda already. > This shit is what I'm talking about: > > Verified Boot off, lockdown on: Perception of security improvement that's > > trivially circumvented (and so bad) > You're doing some serious value judgement that is simply bogus. > If lockdown actually helps avoid CPL0 execution attacks, then it helps > even if secure more is off. Bear in mind that I'm talking about defaults here - in more constrained configurations the answers may change. But the kernel has no way of knowing whether it's in one of those configurations, and as a result there's an argument for not overpromising on the security that you're providing users. If a user has a configuration where you're able to verify that userspace has some degree of protection (eg, disk encryption keys are in the TPM and won't unseal if someone's switched out the kernel) then it's reasonable for userland (or a kernel command line option) to enable the functionality. What I'm afraid of is this turning into a "security" feature that ends up being circumvented in most scenarios where it's currently deployed - eg, module signatures are mostly worthless in the non-lockdown case because you can just grab the sig_enforce symbol address and then kexec a preamble that flips it back to N regardless of the kernel config. This is the sort of thing that's not obvious to most users, and it potentially causes them to make worse security decisions as a result. The goal for this part of the patchset isn't to cover every single conceivable case, it's to provide reasonable defaults in a way that makes life easier for distributions. > Or think of virtual machines - which people often use on purpose for > security things. Again, they very much are _not_ going to have secure > boot, but it's not necessarily even possible to "replace the kernel > and reboot" at all, because the kernel came from outside the virtual > machine entirely, and rebooting might just kill the VM rather than > restart anything. And where you have a trustworthy external thing providing your kernel, yeah, that's also an argument - and having a kernel command line argument that enables it in this case also seems entirely reasonable. > This is what I mean by having an agenda. We all know you are a big > proponent of secure boot. But it seems to cloud your arguments, by > turning your assumptions and your agenda into an "argument" that is > simply not even TRUE. I'm making this argument from the perspective of "What should the kernel do when it has no additional information". Having the kernel automatically enable lockdown without the user being aware of which guarantees their environment is providing risks giving users the impression of security that they may not have - in that case it makes more sense to have the user make an explicit decision to enable it. > See what I'm unhappy about? > > Verified Boot on, lockdown off: Perception of security improvement that's > > trivially circumvented (and so bad), status quo in mainline kernels > I think this is entirely false too. Again, the "trivial circumvention" > shows a bias and agenda that isn't really all that true. > > Of these four options, only two make sense. > No. > You say that, because you have that bias and that agenda. Ok. Only two make sense *in the absence of additional information about local configuration*. Distributions have to make reasonable choices here, and where a configuration choice decreases functionality and provides what may only be a marginal increase in security it's not a good configuration choice to make by default. > That said, wouldn't it be equally good to just make the whole thing be > a protected EFI variable instead? Once you have physical access to the > EFI shell (to turn off secure boot) you have access to that too. That's pretty much exactly what mokutil does, without you needing to find a copy of the UEFI shell to install first. If you think there's a strong enough need for it, we could definitely add an additional variable that allowed you to disable lockdown without disabling signature validation. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:56 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 5:46 PM, Matthew Garrett wrote: > > > > The generic distros have been shipping this policy for the past 5 years. > .. so apparently it doesn't actually break things? Why not enable it > by default then? Because it does break things, and the documented fix is "Disable Secure Boot by running mokutil --disable-validation". > And if "turn off secure boot" really is the accepted - and actuially > used - workaround for the breakage, then > WHY THE HELL DIDN'T YOU START OFF BY EXPLAINING THAT IN THE FIRST > PLACE WHEN PEOPLE ASKED WHY THE TIE-IN EXISTED? > Sorry for shouting, but really. We have a thread of just *how* many > email messages that asked for the explanation for this? All we got was > incomprehensible and illogical crap explanations. There are four cases: Verified Boot off, lockdown off: Status quo in distro and mainline kernels Verified Boot off, lockdown on: Perception of security improvement that's trivially circumvented (and so bad) Verified Boot on, lockdown off: Perception of security improvement that's trivially circumvented (and so bad), status quo in mainline kernels Verified Boot on, lockdown on: Security improvement, status quo in distro kernels Of these four options, only two make sense. The most common implementation of Verified Boot on x86 platforms is UEFI Secure Boot, so this patchset includes an option that (if set) results in the kernel doing the right thing without user intervention. This makes it easy for a user to switch between the two states that make sense by running a single command and then following some prompts on the next reboot. The alternative would be to provide a signed kernel that always enabled lockdown and an unsigned kernel that didn't, which would (a) increase load on distributions and (b) force users to both run mokutil --disable-validation and also install a different kernel. I'm sorry if I've appeared tetchy in this discussion - having several of my coworkers shot has not done wonders for my mood. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:33 PM Linus Torvalds wrote: > In contrast, the generic distros can't enable it anyway if it breaks > random hardware. And it wouldn't be about secure boot or not, but > about the random hardware choice. The generic distros have been shipping this policy for the past 5 years. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:18 PM Andy Lutomirski wrote: > if your secure boot-enabled bootloader can't prevent a bad guy from > using malicious kernel command line parameters, then fix it. How is a bootloader supposed to know what the set of malicious kernel command line parameters is? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:15 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 5:10 PM, Matthew Garrett wrote: > > > >> Exactly like EVERY OTHER KERNEL CONFIG OPTION. > > > > So your argument is that we should make the user experience worse? Without > > some sort of verified boot mechanism, lockdown is just security theater. > > There's no good reason to enable it unless you have some mechanism for > > verifying that you booted something you trust. > Wow. Way to snip the rest of the email where I told you what the > solution was. Let me repeat it here, since you so conveniently missed > it and deleted it: I ignored it because it's not a viable option. Part of the patchset disables various kernel command line options. If there's a kernel command line option that disables the patchset then it's pointless. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:08 PM Linus Torvalds wrote: > Still better than telling them to disable/enable secure boot, which > they may or may not even be able to to. Users who can boot a non-vendor Linux distribution on their platform can disable Secure Boot 100% of the time. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:06 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 4:59 PM, Matthew Garrett wrote: > > > > Ok. So we can build distribution kernels that *always* have this on, and to > > turn it off you have to disable Secure Boot and install a different kernel. > Bingo. > Exactly like EVERY OTHER KERNEL CONFIG OPTION. So your argument is that we should make the user experience worse? Without some sort of verified boot mechanism, lockdown is just security theater. There's no good reason to enable it unless you have some mechanism for verifying that you booted something you trust. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 5:02 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 4:47 PM, Matthew Garrett wrote: > >> Another way of looking at this: if lockdown is a good idea to enable > >> when you booted using secure boot, then why isn't it a good idea when > >> you *didn't* boot using secure boot? > > > > Because it's then trivial to circumvent and the restrictions aren't worth > > the benefit. > Bullshit. > If there those restrictions cause problems, they need to be fixed regardless. How? When there are random DMA-capable PCI devices that are driven by userland tools that are mmap()ing the BARs out of sysfs, how do we simultaneously avoid breaking those devices while also preventing the majority of users from being vulnerable to an attacker just DMAing over the kernel? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 4:55 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 4:45 PM, Matthew Garrett wrote: > >> Be honest now. It wasn't generally users who clamored for it. > > > > If you ask a user whether they want a system that lets an attacker replace > > their kernel or one that doesn't, what do you think their answer is likely > > to be? > Goddamnit. > We both know what the answer will be. > And it will have *nothing* to do with secure boot. Right, because they care about outcome rather than mechanism. Secure Boot is the mechanism we have to make that outcome possible. > > Again, what is your proposed mechanism for ensuring that off the shelf > > systems can be configured in a way that makes this possible? > If you think lockdown is a good idea, and you enabled it, then IT IS ENABLED. Ok. So we can build distribution kernels that *always* have this on, and to turn it off you have to disable Secure Boot and install a different kernel. Or we can build distribution kernels that only have this on when you're booting in a context that makes sense, and you can disable it by just disabling Secure Boot (by running mokutil --disable-validation) and not have to install a new kernel. Which outcome do you prefer? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 4:39 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 4:26 PM, Linus Torvalds > wrote: > > > > Magically changing kernel behavior depending on some subtle and often > > unintentional bootup behavior detail is completely idiotic. > Another way of looking at this: if lockdown is a good idea to enable > when you booted using secure boot, then why isn't it a good idea when > you *didn't* boot using secure boot? Because it's then trivial to circumvent and the restrictions aren't worth the benefit. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 4:26 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 4:17 PM, Matthew Garrett wrote: > > > > 1) Secure Boot is intended to permit the construction of a boot chain that > > only runs ring 0 code that the user considers trustworthy > No. > That may be *one* intention, for some people. > It's not an a-priori one for the actual user. Secure Boot is intended to *permit* that. Without Secure Boot you're unable to do that. Some users want that. Some users don't. > > 2) Allowing arbitrary user code to run in ring 0 without affirmative > > consent on the part of the user is therefore incompatible with the goals of > > Secure Boot > Again, that has absolutely zero relevance. > Those goals are not the *users* goals. > Be honest now. It wasn't generally users who clamored for it. If you ask a user whether they want a system that lets an attacker replace their kernel or one that doesn't, what do you think their answer is likely to be? > If the user actually wanted it, and is asking for it, he can enable > it. Independently of secure boot, which the user generally has little > control over. How? If the bootloader will boot kernels that don't impose this restriction then an attacker just replaces whatever's enabling that feature. And, uh, seriously, I've been asking for *years* for someone to point me at a PC on the market that doesn't give the user control over Secure Boot, but Shim was expressly designed to ensure that the user would have the ability to enroll additional trusted keys (or disable signature validation entirely), so which cases are you thinking of where the user doesn't have control? > > 3) This patchset provides a mechanism to alter the behaviour of the kernel > > such that it is significantly more difficult for arbitrary user code to run > > in ring 0 without affirmative user consent > That difficulty already exists, the new thing isn't somehow related to > that at all. > Look at our "uyou can only load modules if you're root" rules. Or the > "you can only load modules if they are signed". > See a pattern there? They don't magically enable themselves (or > disable themselves) depending on whether you booted with secure boot > or not. What's the benefit of "You can only load modules if they are signed" if root is able to just overwrite that policy bit in the kernel? The split between unprivileged users and root is real, but right now module signatures are theater - there's no significant security benefit from them. But the reason to tie this to Secure Boot is that without that an attacker who has root can just replace the kernel on disk (or patch the bootloader to live-patch the kernel on boot, and yes that's an attack we've seen in the real world), so while it's a feature that is arguably beneficial under all circumstances it's a feature that only has significant benefit if you have some way to actually validate what you're booting in the first place. > > 4) Providing a mechanism for automatically enabling this behaviour when > > running in a context that is intended to restrict access to ring 0 is a > > rational thing to do, because otherwise it is difficult to achieve the > > objective in (1) > No. See why it's *NOT* rational, as explained already several times. > Magically changing kernel behavior depending on some subtle and often > unintentional bootup behavior detail is completely idiotic. > It would be idiotic if it was that "check kernel module signatures" > check. This is no less idiotic. > Seriously, listen to your own arguments. If they don't make sense for > checking kernel module signatures, why the hell would they make sense > for something like lockdown. > THE TWO THINGS ARE ENTIRELY INDEPENDENT. Again, what is your proposed mechanism for ensuring that off the shelf systems can be configured in a way that makes this possible? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 4:08 PM Linus Torvalds wrote: > That's not the right approach to begin with, Matthew. The onus is on > *you* to explain why you tied them together, not on others to explain > to you - over and over - that they have nothing to do with each other. 1) Secure Boot is intended to permit the construction of a boot chain that only runs ring 0 code that the user considers trustworthy 2) Allowing arbitrary user code to run in ring 0 without affirmative consent on the part of the user is therefore incompatible with the goals of Secure Boot 3) This patchset provides a mechanism to alter the behaviour of the kernel such that it is significantly more difficult for arbitrary user code to run in ring 0 without affirmative user consent 4) Providing a mechanism for automatically enabling this behaviour when running in a context that is intended to restrict access to ring 0 is a rational thing to do, because otherwise it is difficult to achieve the objective in (1) Alternative approaches to achieve (1) rely on severely constraining userland - ChromeOS, for instance, doesn't impose these restrictions at present but also doesn't allow users to run arbitrary applications (you're stuck inside either the Chrome or Android sandbox). So, if the goal is to achieve (1) when the platform is in this state, what's a more reasonable alternative? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 3:53 PM Andy Lutomirski wrote: > On Tue, Apr 3, 2018 at 3:51 PM, Matthew Garrett wrote: > > Lockdown is clearly useful without Secure Boot (and I intend to deploy it > > that way for various things), but I still don't understand why you feel > > that the common case of booting a kernel from a boot chain that's widely > > trusted derives no benefit from it being harder to subvert that kernel into > > subverting that boot chain. For cases where you're self-signing and feel > > happy about that, you just set CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT to n and > > everyone's happy? > I would like to see distros that want Secure Boot to annoy users by > enabling Lockdown be honest about the fact that it's an annoyance and > adds very little value by having to carry a patch that was rejected by > the upstream kernel. I disagree with the assertion that it adds very little value, but if you want to reject a technically useful patch for political reasons then I'm well beyond the point of caring. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 3:46 PM Linus Torvalds wrote: > For example, I love signed kernel modules. The fact that I love them > has absolutely zero to do with secure boot, though. There is > absolutely no linkage between the two issues: I use (self-)signed > kernel modules simply because I think it's a good thing in general. > The same thing is true of some lockdown patch. Maybe it's a good thing > in general. But whether it's a good thing is _entirely_ independent of > any secure boot issue. I can see using secure boot without it, but I > can very much also see using lockdown without secure boot. > The two things are simply entirely orthogonal. They have _zero_ > overlap. I'm not seeing why they'd be linked at all in any way. Lockdown is clearly useful without Secure Boot (and I intend to deploy it that way for various things), but I still don't understand why you feel that the common case of booting a kernel from a boot chain that's widely trusted derives no benefit from it being harder to subvert that kernel into subverting that boot chain. For cases where you're self-signing and feel happy about that, you just set CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT to n and everyone's happy? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 2:21 PM Al Viro wrote: > On Tue, Apr 03, 2018 at 09:08:54PM +0000, Matthew Garrett wrote: > > If you don't want Secure Boot, turn it off. If you want Secure Boot, use a > > kernel that behaves in a way that actually increases your security. > That assumes you *can* turn that shit off. On the hardware where manufacturer > has installed firmware that doesn't allow that SB is a misfeature that has > to be worked around. Making that harder might improve the value of SB to > said manufacturers, but what's the benefit for everybody else? This is why Shim has support for its own key database, as well as allowing you to disable further signature validation. If the hardware supports third party code at all, you can just use Shim to sidestep any unreasonable restrictions the vendor has imposed. (This doesn't help with systems that don't support third party code at all, but this patchset does nothing to make that worse - that hardware wouldn't boot your own kernel before this patchset, and it won't afterwards either) -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 2:26 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 2:08 PM, Matthew Garrett wrote: > > > > Secure Boot ensures that the firmware will only load signed bootloaders. If > > a signed bootloader loads a kernel that's effectively an unsigned > > bootloader, there's no point in using Secure Boot > Bullshit. > I may want to know that I'm running *my* kernel, but once that is the > case, I trust it. If you don't believe that your self-signed kernel is going to be a threat against your security model then great! Don't turn this on when you build it. But if you built a kernel that didn't have this lockdown functionality and got it signed with, say, Red Hat's signing keys, anyone could take Red Hat's bootloader chain and that kernel and subvert the Secure Boot chain on any machine that trusts the third party signing key (ie, basically all of them) > Yes, on x86 hardware at least at some point MS actually had the rule > that it has to be something you can turn off. That rule is apparently > not true on ARM, though. Correct - there's no requirement that it be something you can disable on ARM, but since Microsoft won't sign any third-party code for ARM anyway it makes no difference to this discussion. > If you want lockdown, fine, enable it. But what the F*CK does that > have to do with whether you had secure boot or not? Because a kernel signed with a generally trusted key that doesn't implement any lockdown functionality is effectively a bootloader that will load unsigned material on most machines on the market, which reduces the security of users running those machines with Secure Boot enabled. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 2:01 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 1:54 PM, Matthew Garrett wrote: > > > >> .. maybe you don't *want* secure boot, but it's been pushed in your > >> face by people with an agenda? > > > > Then turn it off, or build a self-signed kernel that doesn't do this? > Umm. So you asked a question, and then when you got an answer you said > "don't do that then". > The fact is, some hardware pushes secure boot pretty hard. That has > *nothing* to do with some "lockdown" mode. Secure Boot ensures that the firmware will only load signed bootloaders. If a signed bootloader loads a kernel that's effectively an unsigned bootloader, there's no point in using Secure Boot - you should just turn it off instead, because it's not giving you any meaningful security. Andy's example gives a scenario where by constraining your *userland* sufficiently you can get close to having the same guarantees, but that involves you having a read-only filesystem and takes you even further away from having a general purpose computer. If you don't want Secure Boot, turn it off. If you want Secure Boot, use a kernel that behaves in a way that actually increases your security. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 1:53 PM Linus Torvalds wrote: > On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett wrote: > > On Tue, Apr 3, 2018 at 8:11 AM Andy Lutomirski wrote: > >> Can you explain that much more clearly? I'm asking why booting via > >> UEFI Secure Boot should enable lockdown, and I don't see what this has > >> to do with kexec. And "someone blacklist[ing] your key in the > >> bootloader" sounds like a political issue, not a technical issue. > > > > A kernel that allows users arbitrary access to ring 0 is just an > > overfeatured bootloader. Why would you want secure boot in that case? > .. maybe you don't *want* secure boot, but it's been pushed in your > face by people with an agenda? Then turn it off, or build a self-signed kernel that doesn't do this? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] Kernel lockdown for secure boot
On Tue, Apr 3, 2018 at 9:46 AM Andy Lutomirski wrote: > On Tue, Apr 3, 2018 at 9:29 AM, Matthew Garrett wrote: > > A kernel that allows users arbitrary access to ring 0 is just an > > overfeatured bootloader. Why would you want secure boot in that case? > To get a chain of trust. I can provision a system with some public > keys, stored in UEFI authenticated variables, such that the system > will only boot a signed image. That signed image, can, in turn, load > a signed (or hashed or otherwise verfified) kernel and a verified > initramfs. The initramfs can run a full system from a verified (using > dm-verity or similar) filesystem, for example. Now it's very hard to > persistently attack this system. Chromium OS does something very much > like this, except that it doesn't use UEFI as far as I know. So does > iOS, and so do some Android versions. None of this requires lockdown, > or even a separation between usermode and kernelmode, to work > correctly. One could even do this on an MMU-less system if one really > cared to. More usefully, someone probably has done this using a > unikernel. That's only viable if you're the only person with the ability to sign stuff for your machine - the moment there are generic distributions that your machine trusts, an attacker can use one as a bootloader to compromise your trust chain. Since most UEFI secure boot systems have to trust generic distributions (if you don't trust the third party signing key then your GPU won't post), the ecosystem depends on it not being possible for people to use generic distributions as bootloaders. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html