Re: [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On Thu, Sep 14, 2017 at 12:02:47PM -0700, Jarkko Sakkinen wrote: > On Thu, Sep 14, 2017 at 11:48:54AM -0700, Matthew Garrett wrote: > > On Thu, Sep 14, 2017 at 11:43 AM, Jarkko Sakkinen > >wrote: > > > On Mon, Sep 11, 2017 at 12:00:21PM +0200, Thiebaud Weksteen wrote: > > >> With TPM 2.0 specification, the event logs may only be accessible by > > >> calling an EFI Boot Service. Modify the EFI stub to copy the log area to > > >> a new Linux-specific EFI configuration table so it remains accessible > > >> once booted. > > >> > > >> When calling this service, it is possible to specify the expected format > > >> of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only > > >> the > > >> first format is retrieved. > > >> > > >> Signed-off-by: Thiebaud Weksteen > > > > > > With a quick skim the code change looks good but I remember from > > > Matthew's talk that there was this issue that ExitBootServices() would > > > cause a yet another event? > > > > > > I guess you could manually synthetize that event by reading the PCR > > > values right after ExitBootServices()? > > > > I think that would involve breaking SHA1… the information should be > > You are absolutely right, was not thinking clearly :-) > > > available in the TCG_TREE_FINAL_EVENTS configuration table, so it > > /should/ just be a matter of merging the events from that into the > > event log. > > Right, it is available through runtime services. Why this isn't part > of the patch set? Anyway, I'll try this out out when I get back to Finland. Still before landing this to mainline I think it would make sense to make it complete wouldn't it? /Jarkko -- 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: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage
Missed a comma in cc list in last reply, readd linux-efi list in cc. On 09/14/17 at 04:08pm, Baoquan He wrote: > On 09/14/17 at 03:49pm, Dave Young wrote: > > > > diff --git a/arch/x86/include/asm/uv/uv.h b/arch/x86/include/asm/uv/uv.h > > > > index b5a32231abd8..93d7ad8763ba 100644 > > > > --- a/arch/x86/include/asm/uv/uv.h > > > > +++ b/arch/x86/include/asm/uv/uv.h > > > > @@ -18,6 +18,11 @@ extern void uv_nmi_init(void); > > > > extern void uv_system_init(void); > > > > extern const struct cpumask *uv_flush_tlb_others(const struct cpumask > > > > *cpumask, > > > > const struct > > > > flush_tlb_info *info); > > > > +#include > > > > +static inline int is_early_uv_system(void) > > > > +{ > > > > + return !((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || > > > > !efi.uv_systab); > > > > +} > > > > Thanks for looking into this, Dave! > > > > > Sorry for jumping in late, I have two questions about the patch: > > > > 1) For efi tables, the only invalid value is EFI_INVALID_TABLE_ADDR, and > > efi struct is initialized as EFI_INVALID_TABLE_ADDR by default so no > > need to check "|| !efi.uv_systab". Do we have any UV firmware specific > > assumption that "0" is also possible to be assigned? > > Hmm, in uv_bios_init() it also checks the !efi.uv_systab case. And > EFI_INVALID_TABLE_ADDR checking is earlier, it won't affect the result > if it's EFI_INVALID_TABLE_ADDR. And !efi.uv_systab can make it safer > since it doesn't work either if efi.uv_systab is 0. Mainly it's not > harmful. > > Mike, what's your thought? Should I only check the (efi.uv_systab == > EFI_INVALID_TABLE_ADDR) case? > > > > > 2) It seems adding this function in uv.h for separating this for uv > > system only purpose. But I feel it is better to put it in efi.h instead. > > At the beginning I put it in efi.c, later Mike suggested putting it in > asm/uv/uv.h. You can also find the discussion in below link. > https://patchwork.kernel.org/patch/9732787/ > > Thanks > Baoquan > > > > > uv_systab is already a member of struct efi, it is in efi.h so it is > > natural to check the table exist or not. Then just include efi.h in > > kaslr.c and use the function. > > > > something like drivers/firmware/efi/esrt.c: esrt_table_exists() > > > > Anyway I have no strong opinon, it looks more natural to me though. > > > > > > > > > > #else /* X86_UV */ > > > > > > > > @@ -30,6 +35,7 @@ static inline const struct cpumask * > > > > uv_flush_tlb_others(const struct cpumask *cpumask, > > > > const struct flush_tlb_info *info) > > > > { return cpumask; } > > > > +static inline int is_early_uv_system(void) { return 0; } > > > > > > > > #endif /* X86_UV */ > > > > > > > > -- > > > > 2.5.5 > > > > > > > > Thanks > > Dave -- 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: [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On Thu, Sep 14, 2017 at 11:48:54AM -0700, Matthew Garrett wrote: > On Thu, Sep 14, 2017 at 11:43 AM, Jarkko Sakkinen >wrote: > > On Mon, Sep 11, 2017 at 12:00:21PM +0200, Thiebaud Weksteen wrote: > >> With TPM 2.0 specification, the event logs may only be accessible by > >> calling an EFI Boot Service. Modify the EFI stub to copy the log area to > >> a new Linux-specific EFI configuration table so it remains accessible > >> once booted. > >> > >> When calling this service, it is possible to specify the expected format > >> of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only the > >> first format is retrieved. > >> > >> Signed-off-by: Thiebaud Weksteen > > > > With a quick skim the code change looks good but I remember from > > Matthew's talk that there was this issue that ExitBootServices() would > > cause a yet another event? > > > > I guess you could manually synthetize that event by reading the PCR > > values right after ExitBootServices()? > > I think that would involve breaking SHA1… the information should be You are absolutely right, was not thinking clearly :-) > available in the TCG_TREE_FINAL_EVENTS configuration table, so it > /should/ just be a matter of merging the events from that into the > event log. Right, it is available through runtime services. Why this isn't part of the patch set? /Jrakko /Jarkko -- 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: [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On Thu, Sep 14, 2017 at 11:43 AM, Jarkko Sakkinenwrote: > On Mon, Sep 11, 2017 at 12:00:21PM +0200, Thiebaud Weksteen wrote: >> With TPM 2.0 specification, the event logs may only be accessible by >> calling an EFI Boot Service. Modify the EFI stub to copy the log area to >> a new Linux-specific EFI configuration table so it remains accessible >> once booted. >> >> When calling this service, it is possible to specify the expected format >> of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only the >> first format is retrieved. >> >> Signed-off-by: Thiebaud Weksteen > > With a quick skim the code change looks good but I remember from > Matthew's talk that there was this issue that ExitBootServices() would > cause a yet another event? > > I guess you could manually synthetize that event by reading the PCR > values right after ExitBootServices()? I think that would involve breaking SHA1… the information should be available in the TCG_TREE_FINAL_EVENTS configuration table, so it /should/ just be a matter of merging the events from that into the event log. -- 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: [PATCH v2 3/3] tpm: parse TPM event logs based on EFI table
On Mon, Sep 11, 2017 at 12:00:22PM +0200, Thiebaud Weksteen wrote: > If we are not able to retrieve the TPM event logs from the ACPI table, > check the EFI configuration table (Linux-specific GUID). > > The format version of the log may be returned by the function. If not > specified (by previous implementation: tpm_acpi and tpm_of), we default > to the version of the chip (previous behaviour). > > Signed-off-by: Thiebaud WeksteenYou saw my comment about file naming. I.e. tpm_eventlog_efi.c would be a more senseful name. > --- > drivers/char/tpm/Makefile| 2 +- > drivers/char/tpm/tpm.h | 8 + > drivers/char/tpm/tpm1_eventlog.c | 15 +++-- > drivers/char/tpm/tpm_efi.c | 66 > > drivers/firmware/efi/efi.c | 2 ++ > include/linux/efi.h | 1 + > 6 files changed, 90 insertions(+), 4 deletions(-) > create mode 100644 drivers/char/tpm/tpm_efi.c > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile > index 23681f01f95a..74182a63eef2 100644 > --- a/drivers/char/tpm/Makefile > +++ b/drivers/char/tpm/Makefile > @@ -4,7 +4,7 @@ > obj-$(CONFIG_TCG_TPM) += tpm.o > tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ >tpm-dev-common.o tpmrm-dev.o tpm1_eventlog.o tpm2_eventlog.o \ > - tpm2-space.o > + tpm2-space.o tpm_efi.o > tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_acpi.o > tpm-$(CONFIG_OF) += tpm_of.o > obj-$(CONFIG_TCG_TIS_CORE) += tpm_tis_core.o > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 46caccf6fd1a..1bd97e01df50 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -597,6 +597,14 @@ static inline int tpm_read_log_of(struct tpm_chip *chip) > return -ENODEV; > } > #endif > +#if defined(CONFIG_EFI) > +int tpm_read_log_efi(struct tpm_chip *chip); > +#else > +static inline int tpm_read_log_efi(struct tpm_chip *chip) > +{ > + return -ENODEV; > +} > +#endif > > int tpm_bios_log_setup(struct tpm_chip *chip); > void tpm_bios_log_teardown(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm1_eventlog.c > b/drivers/char/tpm/tpm1_eventlog.c > index d6f70f365443..7e25e6bff6ce 100644 > --- a/drivers/char/tpm/tpm1_eventlog.c > +++ b/drivers/char/tpm/tpm1_eventlog.c > @@ -21,6 +21,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -371,6 +372,10 @@ static int tpm_read_log(struct tpm_chip *chip) > if (rc != -ENODEV) > return rc; > > + rc = tpm_read_log_efi(chip); > + if (rc != -ENODEV) > + return rc; > + > return tpm_read_log_of(chip); > } > > @@ -388,11 +393,13 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > { > const char *name = dev_name(>dev); > unsigned int cnt; > - int rc = 0; > + int rc = 0, log_version; A tid bit, one declaration per line. > + > > rc = tpm_read_log(chip); > - if (rc) > + if (rc < 0) > return rc; > + log_version = rc; > > cnt = 0; > chip->bios_dir[cnt] = securityfs_create_dir(name, NULL); > @@ -404,7 +411,9 @@ int tpm_bios_log_setup(struct tpm_chip *chip) > cnt++; > > chip->bin_log_seqops.chip = chip; > - if (chip->flags & TPM_CHIP_FLAG_TPM2) > + > + if (log_version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 || > + (!log_version && (chip->flags & TPM_CHIP_FLAG_TPM2))) > chip->bin_log_seqops.seqops = > _binary_b_measurements_seqops; > else > diff --git a/drivers/char/tpm/tpm_efi.c b/drivers/char/tpm/tpm_efi.c > new file mode 100644 > index ..c8247fc45bb0 > --- /dev/null > +++ b/drivers/char/tpm/tpm_efi.c > @@ -0,0 +1,66 @@ > +/* > + * Copyright (C) 2017 Google > + * > + * Authors: > + * Thiebaud Weksteen > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + */ > + > +#include > +#include > + > +#include "tpm.h" > + > +/* read binary bios log from EFI configuration table */ > +int tpm_read_log_efi(struct tpm_chip *chip) > +{ > + > + struct linux_efi_tpm_eventlog *log_tbl; > + struct tpm_bios_log *log; > + u32 log_size; > + u8 tpm_log_version; > + > + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) > + return -ENODEV; > + > + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) > + return -ENODEV; > + > + log = >log; > + > + log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl), MEMREMAP_WB); > + if (!log_tbl) { > + pr_err("Could not map UEFI TPM log table !\n"); > + return -ENOMEM; > + } > + > + log_size = log_tbl->size; > + iounmap(log_tbl); > + > + log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl) +
Re: [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On Mon, Sep 11, 2017 at 12:00:21PM +0200, Thiebaud Weksteen wrote: > With TPM 2.0 specification, the event logs may only be accessible by > calling an EFI Boot Service. Modify the EFI stub to copy the log area to > a new Linux-specific EFI configuration table so it remains accessible > once booted. > > When calling this service, it is possible to specify the expected format > of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only the > first format is retrieved. > > Signed-off-by: Thiebaud WeksteenWith a quick skim the code change looks good but I remember from Matthew's talk that there was this issue that ExitBootServices() would cause a yet another event? I guess you could manually synthetize that event by reading the PCR values right after ExitBootServices()? Anyway, great work, thanks for making this effort. /Jarkko -- 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: [tpmdd-devel] [PATCH v2 2/3] efi: call get_event_log before ExitBootServices
On 09/11/2017 12:00 PM, Thiebaud Weksteen via tpmdd-devel wrote: > With TPM 2.0 specification, the event logs may only be accessible by > calling an EFI Boot Service. Modify the EFI stub to copy the log area to > a new Linux-specific EFI configuration table so it remains accessible > once booted. > > When calling this service, it is possible to specify the expected format > of the logs: TPM 1.2 (SHA1) or TPM 2.0 ("Crypto Agile"). For now, only the > first format is retrieved. > > Signed-off-by: Thiebaud Weksteen> --- [snip] > +void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg) > +{ [snip] > + > + /* Allocate space for the logs and copy them. */ > + status = efi_call_early(allocate_pool, EFI_LOADER_DATA, > + sizeof(*log_tbl) + log_size, > + (void **) _tbl); > + > + if (status != EFI_SUCCESS) { > + efi_printk(sys_table_arg, > +"Unable to allocate memory for event log\n"); > + return; > + } If this fails or any previous error that will prevent the event log table + logs to be allocated, shouldn't tpm_read_log_efi() be notified somehow? Since AFAICT it will still try to access them even if the EFI allocate_pool did not succeed. > + */ > +int __init efi_tpm_eventlog_init(void) > +{ > + struct linux_efi_tpm_eventlog *tbl; > + unsigned int tbl_size; > + The functions efi_retrieve_tpm2_eventlog_1_2() and tpm_read_log_efi() are using log_tbl as variable name, so I would use it here too for consistency. > + if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) > + return 0; > + > + tbl = early_memremap(efi.tpm_log, sizeof(*tbl)); > + if (!tbl) { > + pr_err("Failed to map TPM Event Log table @ 0x%lx\n", > + efi.tpm_log); > + return -ENOMEM; > + } > + Same question than before, if this fails then the table + logs memory won't be reserved but tpm_read_log_efi() will still try to access it. I'm not sure what is the correct way to notify though, maybe setting efi.tpm_log to 0 and then in tpm_read_log_efi() check efi.tpm_log for 0 or EFI_INVALID_TABLE_ADDR instead? > + tbl_size = sizeof(*tbl) + tbl->size; > + memblock_reserve(efi.tpm_log, tbl_size); > + early_memunmap(tbl, sizeof(*tbl)); > + return 0; Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat -- 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: [tpmdd-devel] [PATCH v2 0/3] Call GetEventLog before ExitBootServices
Hello Thiebaud, On 09/11/2017 12:00 PM, Thiebaud Weksteen via tpmdd-devel wrote: > With TPM 1.2, the ACPI table ("TCPA") has two fields to recover the Event Log > Area (LAML and LASA). These logs are useful to understand and rebuild the > final values of PCRs. > > With TPM 2.0, the ACPI table ("TPM2") does not contain these fields anymore. > The recommended method is now to call the GetEventLog EFI protocol before > ExitBootServices. > > Implement this method within the EFI stub and create copy of the logs for the > TPM device. This will create > /sys/kernel/security/tpm0/binary_bios_measurements > for TPM 2.0 devices (similarly to the current behaviour for TPM 1.2 devices). > I've tested your patches on a system with an Intel PTT firmware based TPM2.0 and the measurements securityfs entry was correctly created and was able to read it: $ cat /sys/class/tpm/tpm0/device/description TPM 2.0 Device $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements | head -n2 000 0008 f504 15a0 1810 bf44 010 63d0 4fdb b8a4 f278 8dc7 c8aa 0014 So please feel free to add: Tested-by: Javier Martinez CanillasI also reviewed the patches and look good to me, I have just one question for patch #2, but I'll comment there. Best regards, -- Javier Martinez Canillas Software Engineer - Desktop Hardware Enablement Red Hat -- 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