Re: [PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN

[Matthew Garrett]

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.

[Matthew Garrett]

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

[Matthew Garrett]

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.

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

(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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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()

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

Identical to V4, but based on tpmdd-next

Re: [PATCH V4 2/4] tpm: Reserve the TPM final events table

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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)

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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

[Matthew Garrett]

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