Re: [PATCH 2/5] MODSIGN: print appropriate status message when getting UEFI certificates list

[joeyli]

Hi James,

Thanks for your review.

On Tue, Mar 13, 2018 at 10:17:50AM -0700, James Bottomley wrote:
> On Tue, 2018-03-13 at 18:35 +0800, Lee, Chun-Yi wrote:
> > When getting certificates list from UEFI variable, the original error
> > message shows the state number from UEFI firmware. It's hard to be
> > read by human. This patch changed the error message to show the
> > appropriate string.
> > 
> > The message will be showed as:
> > 
> > [0.788529] MODSIGN: Couldn't get UEFI MokListRT: EFI_NOT_FOUND
> > [0.788537] MODSIGN: Couldn't get UEFI MokListXRT: EFI_NOT_FOUND
> 
> I keep saying this, but these error messages need to be gated on the
> presence of shim for the non-shim secure boot case.  You can't assume
> the shim variables are there because they won't be in the case of a
> fully owned system.

I will change the message to pr_debug.

Thanks a lot!
Joey Lee 
--
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 1/5] MODSIGN: do not load mok when secure boot disabled

[Ard Biesheuvel]

On 13 March 2018 at 10:37, Lee, Chun-Yi  wrote:
> The mok can not be trusted when the secure boot is disabled. Which
> means that the kernel embedded certificate is the only trusted key.
>
> Due to db/dbx are authenticated variables, they needs manufacturer's
> KEK for update. So db/dbx are secure when secureboot disabled.
>

Did you consider the case where secure boot is not implemented? I
don't think db/dbx are secure in that case, although perhaps it may
not matter (a bit more information on the purpose of these patches and
all the shim lingo etc would be appreciated)

> Cc: David Howells 
> Cc: Josh Boyer 
> Cc: James Bottomley 
> Signed-off-by: "Lee, Chun-Yi" 
> ---
>  certs/load_uefi.c | 26 +++---
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
> index 3d88459..d6de4d0 100644
> --- a/certs/load_uefi.c
> +++ b/certs/load_uefi.c
> @@ -164,17 +164,6 @@ static int __init load_uefi_certs(void)
> }
> }
>
> -   mok = get_cert_list(L"MokListRT", _var, );

Which tree does this apply to? My tree doesn't have get_cert_list()

> -   if (!mok) {
> -   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
> -   } else {
> -   rc = parse_efi_signature_list("UEFI:MokListRT",
> - mok, moksize, 
> get_handler_for_db);
> -   if (rc)
> -   pr_err("Couldn't parse MokListRT signatures: %d\n", 
> rc);
> -   kfree(mok);
> -   }
> -
> dbx = get_cert_list(L"dbx", _var, );
> if (!dbx) {
> pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
> @@ -187,6 +176,21 @@ static int __init load_uefi_certs(void)
> kfree(dbx);
> }
>
> +   /* the MOK can not be trusted when secure boot is disabled */
> +   if (!efi_enabled(EFI_SECURE_BOOT))
> +   return 0;
> +
> +   mok = get_cert_list(L"MokListRT", _var, );
> +   if (!mok) {
> +   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
> +   } else {
> +   rc = parse_efi_signature_list("UEFI:MokListRT",
> + mok, moksize, 
> get_handler_for_db);
> +   if (rc)
> +   pr_err("Couldn't parse MokListRT signatures: %d\n", 
> rc);
> +   kfree(mok);
> +   }
> +
> return rc;
>  }
>  late_initcall(load_uefi_certs);
> --
> 2.10.2
>
> --
> 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
--
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 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[James Bottomley]

On Tue, 2018-03-13 at 18:38 +0800, Lee, Chun-Yi wrote:
> This patch adds the logic for checking the kernel module's hash
> base on blacklist. The hash must be generated by sha256 and enrolled
> to dbx/mokx.
> 
> For example:
>   sha256sum sample.ko
>   mokutil --mokx --import-hash $HASH_RESULT
> 
> Whether the signature on ko file is stripped or not, the hash can be
> compared by kernel.

What's the use case for this?  We're already in trouble from the ODMs
for the size of dbx and its consumption of the extremely limited
variable space, so do we really have a use case for adding module
blacklist hashes to the UEFI variables given the space constraints (as
in one we can't do any other way)?

James

--
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 2/5] MODSIGN: print appropriate status message when getting UEFI certificates list

[James Bottomley]

On Tue, 2018-03-13 at 18:35 +0800, Lee, Chun-Yi wrote:
> When getting certificates list from UEFI variable, the original error
> message shows the state number from UEFI firmware. It's hard to be
> read by human. This patch changed the error message to show the
> appropriate string.
> 
> The message will be showed as:
> 
> [0.788529] MODSIGN: Couldn't get UEFI MokListRT: EFI_NOT_FOUND
> [0.788537] MODSIGN: Couldn't get UEFI MokListXRT: EFI_NOT_FOUND

I keep saying this, but these error messages need to be gated on the
presence of shim for the non-shim secure boot case.  You can't assume
the shim variables are there because they won't be in the case of a
fully owned system.

James

--
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 1/1] efi/libstub: tpm: zero initialize pointer variables for mixed mode

[Javier Martinez Canillas]

[adding linux-integrity and tpmdd-devel since this was discussed in these ML 
too]

On 03/13/2018 03:09 PM, Ard Biesheuvel wrote:
> As reported by Jeremy, running the new TPM libstub code in mixed mode
> (i.e., 64-bit kernel on 32-bit UEFI) results in hangs when invoking
> the TCG2 protocol, or when accessing the log_tbl pool allocation.
> 
> The reason turns out to be that in both cases, the 64-bit pointer
> variables are not fully initialized by the 32-bit EFI code, and so
> we should take care to zero initialize these variables beforehand,
> or we'll end up dereferencing bogus pointers.
> 
> Reported-by: Jeremy Cline 
> Signed-off-by: Ard Biesheuvel 
> ---
>  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 da661bf8cb96..13c1edd37e96 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -68,11 +68,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
> *sys_table_arg)
>   efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID;
>   efi_status_t status;
>   efi_physical_addr_t log_location, log_last_entry;
> - struct linux_efi_tpm_eventlog *log_tbl;
> + struct linux_efi_tpm_eventlog *log_tbl = NULL;
>   unsigned long first_entry_addr, last_entry_addr;
>   size_t log_size, last_entry_size;
>   efi_bool_t truncated;
> - void *tcg2_protocol;
> + void *tcg2_protocol = NULL;
>  
>   status = efi_call_early(locate_protocol, _guid, NULL,
>   _protocol);
> 

Looks good to me.

Reviewed-by: Javier Martinez Canillas 

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

[GIT PULL 0/1] EFI fix for v4.16

[Ard Biesheuvel]

The following changes since commit 7928b2cbe55b2a410a0f5c1f154610059c57b1b2:

  Linux 4.16-rc1 (2018-02-11 15:04:29 -0800)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

for you to fetch changes up to 5c4d724b7ccfa934b95c5c49fab770035a425950:

  efi/libstub: tpm: zero initialize pointer variables for mixed mode 
(2018-03-13 13:48:25 +)


EFI fix for v4.16:
- fix an issue in the mixed mode handling of the new TPM libstub code


Ard Biesheuvel (1):
  efi/libstub: tpm: zero initialize pointer variables for mixed mode

 drivers/firmware/efi/libstub/tpm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
--
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

[PATCH 1/1] efi/libstub: tpm: zero initialize pointer variables for mixed mode

[Ard Biesheuvel]

As reported by Jeremy, running the new TPM libstub code in mixed mode
(i.e., 64-bit kernel on 32-bit UEFI) results in hangs when invoking
the TCG2 protocol, or when accessing the log_tbl pool allocation.

The reason turns out to be that in both cases, the 64-bit pointer
variables are not fully initialized by the 32-bit EFI code, and so
we should take care to zero initialize these variables beforehand,
or we'll end up dereferencing bogus pointers.

Reported-by: Jeremy Cline 
Signed-off-by: Ard Biesheuvel 
---
 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 da661bf8cb96..13c1edd37e96 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -68,11 +68,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID;
efi_status_t status;
efi_physical_addr_t log_location, log_last_entry;
-   struct linux_efi_tpm_eventlog *log_tbl;
+   struct linux_efi_tpm_eventlog *log_tbl = NULL;
unsigned long first_entry_addr, last_entry_addr;
size_t log_size, last_entry_size;
efi_bool_t truncated;
-   void *tcg2_protocol;
+   void *tcg2_protocol = NULL;
 
status = efi_call_early(locate_protocol, _guid, NULL,
_protocol);
-- 
2.15.1

--
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: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 13:41, Jeremy Cline  wrote:
> On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
>> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>>> Hi,
>>>
>>>
>>> On 12-03-18 20:55, Thiebaud Weksteen wrote:

>> ...

 Hans, you said you configured the tablet to use the 32-bit version of grub
 instead
 of 64. Why's that?
>>>
>>>
>>> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
>>> UEFI,
>>> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers
>>> were
>>> not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
>>>
>>> So this is running a 32 bit grub which boots a 64 bit kernel.
>>>
 Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
 your Android install working? (that is, what happens if you boot
 Boot)?
>>>
>>>
>>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>>
>>> Could the problem perhaps be that the new code for the TPM event-log is
>>> missing some handling to deal with running on a 32 bit firmware? I know the
>>> rest of the kernel has special code to deal with this.
>>>
>>
>> That is a very good point, and I missed that this is a 64-bit kernel
>> running on 32-bit UEFI.
>>
>> The TPM code does use efi_call_proto() directly, and now I am thinking
>> it is perhaps the allocate_pages() call that simply only initializes
>> the low 32-bits of log_tbl.
>>
>> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
>> NULL at the start of the function?
>>
>
> That was it, it boots when those are initialized to NULL.
>

Thanks for confirming. I'll send out a patch.
--
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: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>> Hi,
>>
>>
>> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>>
> ...
>>>
>>> Hans, you said you configured the tablet to use the 32-bit version of grub
>>> instead
>>> of 64. Why's that?
>>
>>
>> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
>> UEFI,
>> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers
>> were
>> not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
>>
>> So this is running a 32 bit grub which boots a 64 bit kernel.
>>
>>> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
>>> your Android install working? (that is, what happens if you boot
>>> Boot)?
>>
>>
>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>
>> Could the problem perhaps be that the new code for the TPM event-log is
>> missing some handling to deal with running on a 32 bit firmware? I know the
>> rest of the kernel has special code to deal with this.
>>
> 
> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.
> 
> The TPM code does use efi_call_proto() directly, and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.
> 
> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
> 

That was it, it boots when those are initialized to NULL.

Regards,
Jeremy
--
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: Regression from efi: call get_event_log before ExitBootServices

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 9:47 AM, Hans de Goede  wrote:
> On 12-03-18 20:55, Thiebaud Weksteen wrote:

>> Hans, you said you configured the tablet to use the 32-bit version of grub
>> instead
>> of 64. Why's that?

> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
> UEFI,
> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers
> were
> not ready in time so all Bay Trail devices shipped with a 32 bit Windows).

Almost.

Lenovo Thinkpad 10 tablet has 64-bit firmware.

-- 
With Best Regards,
Andy Shevchenko
--
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

[PATCH 0/5 v2] Using the hash in MOKx to blacklist kernel module

[Lee, Chun-Yi]

This patch set is base on the efi-lock-down and keys-uefi branchs in
David Howells's linux-fs git tree.

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-uefi

The main purpose is using the MOKx to blacklist kernel module.

As the MOK (Machine Owner Key), MOKx is a EFI boot time variable which
is maintained by shim boot loader. We can enroll the hash of blacklisted
kernel module (with or without signature) to MOKx by mokutil. Kernel loads
the hash from MOKx to blacklist keyring when booting. Kernel will prevent
to load the kernel module when its hash be found in blacklist.

This function is useful to revoke a kernel module that it has exploit. Or
revoking a kernel module that it was signed by a unsecure key.

Except MOKx, this patch set fixs another two issues: The MOK/MOKx should
not be loaded when secure boot is disabled. And, modified error message
prints out appropriate status string for reading by human being.

v2:
Chekcikng the attributes of db and mok before loading certificates.

Lee, Chun-Yi (5):
  MODSIGN: do not load mok when secure boot disabled
  MODSIGN: print appropriate status message when getting UEFI
certificates list
  MODSIGN: load blacklist from MOKx
  MODSIGN: checking the blacklisted hash before loading a kernel module
  MODSIGN: check the attributes of db and mok

 certs/load_uefi.c   | 92 +++--
 include/linux/efi.h | 25 ++
 kernel/module_signing.c | 62 +++--
 3 files changed, 152 insertions(+), 27 deletions(-)

-- 
2.10.2

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

[PATCH 3/5] MODSIGN: load blacklist from MOKx

[Lee, Chun-Yi]

This patch adds the logic to load the blacklisted hash and
certificates from MOKx which is maintained by shim bootloader.

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: "Lee, Chun-Yi" 
---
 certs/load_uefi.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index f2f372b..dc66a79 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -164,8 +164,8 @@ static int __init load_uefi_certs(void)
 {
efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
-   void *db = NULL, *dbx = NULL, *mok = NULL;
-   unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+   void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL;
+   unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0;
int rc = 0;
 
if (!efi.get_variable)
@@ -195,7 +195,7 @@ static int __init load_uefi_certs(void)
kfree(dbx);
}
 
-   /* the MOK can not be trusted when secure boot is disabled */
+   /* the MOK and MOKx can not be trusted when secure boot is disabled */
if (!efi_enabled(EFI_SECURE_BOOT))
return 0;
 
@@ -208,6 +208,16 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
 
+   mokx = get_cert_list(L"MokListXRT", _var, );
+   if (mokx) {
+   rc = parse_efi_signature_list("UEFI:mokx",
+ mokx, mokxsize,
+ get_handler_for_dbx);
+   if (rc)
+   pr_err("Couldn't parse MokListXRT signatures: %d\n", 
rc);
+   kfree(mokx);
+   }
+
return rc;
 }
 late_initcall(load_uefi_certs);
-- 
2.10.2

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

[PATCH 2/5] MODSIGN: print appropriate status message when getting UEFI certificates list

[Lee, Chun-Yi]

When getting certificates list from UEFI variable, the original error
message shows the state number from UEFI firmware. It's hard to be read
by human. This patch changed the error message to show the appropriate
string.

The message will be showed as:

[0.788529] MODSIGN: Couldn't get UEFI MokListRT: EFI_NOT_FOUND
[0.788537] MODSIGN: Couldn't get UEFI MokListXRT: EFI_NOT_FOUND

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: "Lee, Chun-Yi" 
---
 certs/load_uefi.c   | 43 ++-
 include/linux/efi.h | 25 +
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index d6de4d0..f2f372b 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -4,6 +4,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "internal.h"
@@ -32,6 +33,24 @@ static __init bool uefi_check_ignore_db(void)
return status == EFI_SUCCESS;
 }
 
+static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t 
status)
+{
+   char *utf8_str;
+   unsigned long utf8_size;
+
+   if (!char16_str)
+   return;
+   utf8_size = ucs2_utf8size(char16_str) + 1;
+   utf8_str = kmalloc(utf8_size, GFP_KERNEL);
+   if (!utf8_str)
+   return;
+   ucs2_as_utf8(utf8_str, char16_str, utf8_size);
+
+   pr_info("MODSIGN: Couldn't get UEFI %s: %s\n",
+   utf8_str, efi_status_to_str(status));
+   kfree(utf8_str);
+}
+
 /*
  * Get a certificate list blob from the named EFI variable.
  */
@@ -45,25 +64,29 @@ static __init void *get_cert_list(efi_char16_t *name, 
efi_guid_t *guid,
 
status = efi.get_variable(name, guid, NULL, , );
if (status != EFI_BUFFER_TOO_SMALL) {
-   pr_err("Couldn't get size: 0x%lx\n", status);
-   return NULL;
+   if (status != EFI_NOT_FOUND)
+   pr_err("Couldn't get size: 0x%lx\n", status);
+   goto err;
}
 
db = kmalloc(lsize, GFP_KERNEL);
if (!db) {
pr_err("Couldn't allocate memory for uefi cert list\n");
-   return NULL;
+   goto err;
}
 
status = efi.get_variable(name, guid, NULL, , db);
if (status != EFI_SUCCESS) {
kfree(db);
pr_err("Error reading db var: 0x%lx\n", status);
-   return NULL;
+   goto err;
}
 
*size = lsize;
return db;
+err:
+   print_get_fail(name, status);
+   return NULL;
 }
 
 /*
@@ -153,9 +176,7 @@ static int __init load_uefi_certs(void)
 */
if (!uefi_check_ignore_db()) {
db = get_cert_list(L"db", _var, );
-   if (!db) {
-   pr_err("MODSIGN: Couldn't get UEFI db list\n");
-   } else {
+   if (db) {
rc = parse_efi_signature_list("UEFI:db",
  db, dbsize, 
get_handler_for_db);
if (rc)
@@ -165,9 +186,7 @@ static int __init load_uefi_certs(void)
}
 
dbx = get_cert_list(L"dbx", _var, );
-   if (!dbx) {
-   pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
-   } else {
+   if (dbx) {
rc = parse_efi_signature_list("UEFI:dbx",
  dbx, dbxsize,
  get_handler_for_dbx);
@@ -181,9 +200,7 @@ static int __init load_uefi_certs(void)
return 0;
 
mok = get_cert_list(L"MokListRT", _var, );
-   if (!mok) {
-   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
-   } else {
+   if (mok) {
rc = parse_efi_signature_list("UEFI:MokListRT",
  mok, moksize, get_handler_for_db);
if (rc)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2729d6f..c44946c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1600,4 +1600,29 @@ struct linux_efi_random_seed {
u8  bits[];
 };
 
+#define EFI_STATUS_STR(_status)\
+   case EFI_##_status: \
+   return "EFI_" __stringify(_status); \
+
+static inline char *
+efi_status_to_str(efi_status_t status)
+{
+   switch (status) {
+   EFI_STATUS_STR(SUCCESS)
+   EFI_STATUS_STR(LOAD_ERROR)
+   EFI_STATUS_STR(INVALID_PARAMETER)
+   EFI_STATUS_STR(UNSUPPORTED)
+   EFI_STATUS_STR(BAD_BUFFER_SIZE)
+   EFI_STATUS_STR(BUFFER_TOO_SMALL)
+   EFI_STATUS_STR(NOT_READY)
+   EFI_STATUS_STR(DEVICE_ERROR)
+   EFI_STATUS_STR(WRITE_PROTECTED)
+   EFI_STATUS_STR(OUT_OF_RESOURCES)
+   EFI_STATUS_STR(NOT_FOUND)
+   

[PATCH 4/5] MODSIGN: checking the blacklisted hash before loading a kernel module

[Lee, Chun-Yi]

This patch adds the logic for checking the kernel module's hash
base on blacklist. The hash must be generated by sha256 and enrolled
to dbx/mokx.

For example:
sha256sum sample.ko
mokutil --mokx --import-hash $HASH_RESULT

Whether the signature on ko file is stripped or not, the hash can be
compared by kernel.

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: "Lee, Chun-Yi" 
---
 kernel/module_signing.c | 62 +++--
 1 file changed, 60 insertions(+), 2 deletions(-)

diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index d3d6f95..d30ac74 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,9 +11,12 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
+#include 
+#include 
 #include "module-internal.h"
 
 enum pkey_id_type {
@@ -42,19 +45,67 @@ struct module_signature {
__be32  sig_len;/* Length of signature data */
 };
 
+static int mod_is_hash_blacklisted(const void *mod, size_t verifylen)
+{
+   struct crypto_shash *tfm;
+   struct shash_desc *desc;
+   size_t digest_size, desc_size;
+   u8 *digest;
+   int ret = 0;
+
+   tfm = crypto_alloc_shash("sha256", 0, 0);
+   if (IS_ERR(tfm))
+   goto error_return;
+
+   desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+   digest_size = crypto_shash_digestsize(tfm);
+   digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
+   if (!digest) {
+   pr_err("digest memory buffer allocate fail\n");
+   ret = -ENOMEM;
+   goto error_digest;
+   }
+   desc = (void *)digest + digest_size;
+   desc->tfm = tfm;
+   desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
+   ret = crypto_shash_init(desc);
+   if (ret < 0)
+   goto error_shash;
+
+   ret = crypto_shash_finup(desc, mod, verifylen, digest);
+   if (ret < 0)
+   goto error_shash;
+
+   pr_debug("%ld digest: %*phN\n", verifylen, (int) digest_size, digest);
+
+   ret = is_hash_blacklisted(digest, digest_size, "bin");
+   if (ret == -EKEYREJECTED)
+   pr_err("Module hash %*phN is blacklisted\n",
+  (int) digest_size, digest);
+
+error_shash:
+   kfree(digest);
+error_digest:
+   crypto_free_shash(tfm);
+error_return:
+   return ret;
+}
+
 /*
  * Verify the signature on a module.
  */
 int mod_verify_sig(const void *mod, unsigned long *_modlen)
 {
struct module_signature ms;
-   size_t modlen = *_modlen, sig_len;
+   size_t modlen = *_modlen, sig_len, wholelen;
+   int ret;
 
pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
if (modlen <= sizeof(ms))
return -EBADMSG;
 
+   wholelen = modlen + sizeof(MODULE_SIG_STRING) - 1;
memcpy(, mod + (modlen - sizeof(ms)), sizeof(ms));
modlen -= sizeof(ms);
 
@@ -80,7 +131,14 @@ int mod_verify_sig(const void *mod, unsigned long *_modlen)
return -EBADMSG;
}
 
-   return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
+   ret = verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
  (void *)1UL, VERIFYING_MODULE_SIGNATURE,
  NULL, NULL);
+   pr_devel("verify_pkcs7_signature() = %d\n", ret);
+
+   /* checking hash of module is in blacklist */
+   if (!ret)
+   ret = mod_is_hash_blacklisted(mod, wholelen);
+
+   return ret;
 }
-- 
2.10.2

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

[PATCH 5/5] MODSIGN: check the attributes of db and mok

[Lee, Chun-Yi]

That's better for checking the attributes of db and mok variables
before loading certificates to kernel keyring.

For db and dbx, both of them are authenticated variables. Which
means that they can only be modified by manufacturer's key. So
the kernel should checks EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS
attribute before we trust it.

For mok-rt and mokx-rt, both of them are created by shim boot loader
to forward the mok/mokx content to runtime. They must be runtime-volatile
variables. So kernel should checks that the attributes map did not set
EFI_VARIABLE_NON_VOLATILE bit before we trust it.

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: "Lee, Chun-Yi" 
---
 certs/load_uefi.c | 35 +++
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index dc66a79..52526bd 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -33,7 +33,8 @@ static __init bool uefi_check_ignore_db(void)
return status == EFI_SUCCESS;
 }
 
-static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t 
status)
+static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t 
status,
+ u32 attr)
 {
char *utf8_str;
unsigned long utf8_size;
@@ -46,8 +47,8 @@ static __init void print_get_fail(efi_char16_t *char16_str, 
efi_status_t status)
return;
ucs2_as_utf8(utf8_str, char16_str, utf8_size);
 
-   pr_info("MODSIGN: Couldn't get UEFI %s: %s\n",
-   utf8_str, efi_status_to_str(status));
+   pr_info("MODSIGN: Couldn't get UEFI %s: %sAttributes: 0x%016x\n",
+   utf8_str, efi_status_to_str(status), attr);
kfree(utf8_str);
 }
 
@@ -55,12 +56,13 @@ static __init void print_get_fail(efi_char16_t *char16_str, 
efi_status_t status)
  * Get a certificate list blob from the named EFI variable.
  */
 static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, u32 pos_attr, u32 
neg_attr)
 {
efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
void *db;
+   u32 attr = 0;
 
status = efi.get_variable(name, guid, NULL, , );
if (status != EFI_BUFFER_TOO_SMALL) {
@@ -75,17 +77,22 @@ static __init void *get_cert_list(efi_char16_t *name, 
efi_guid_t *guid,
goto err;
}
 
-   status = efi.get_variable(name, guid, NULL, , db);
+   status = efi.get_variable(name, guid, , , db);
if (status != EFI_SUCCESS) {
-   kfree(db);
pr_err("Error reading db var: 0x%lx\n", status);
-   goto err;
+   goto free;
}
+   /* must have positive attributes and no negative attributes */
+   if ((pos_attr && !(attr & pos_attr)) ||
+   (neg_attr && (attr & neg_attr)))
+   goto free;
 
*size = lsize;
return db;
+free:
+   kfree(db);
 err:
-   print_get_fail(name, status);
+   print_get_fail(name, status, attr);
return NULL;
 }
 
@@ -175,7 +182,8 @@ static int __init load_uefi_certs(void)
 * an error if we can't get them.
 */
if (!uefi_check_ignore_db()) {
-   db = get_cert_list(L"db", _var, );
+   db = get_cert_list(L"db", _var, ,
+   EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, 0);
if (db) {
rc = parse_efi_signature_list("UEFI:db",
  db, dbsize, 
get_handler_for_db);
@@ -185,7 +193,8 @@ static int __init load_uefi_certs(void)
}
}
 
-   dbx = get_cert_list(L"dbx", _var, );
+   dbx = get_cert_list(L"dbx", _var, ,
+   EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS, 0);
if (dbx) {
rc = parse_efi_signature_list("UEFI:dbx",
  dbx, dbxsize,
@@ -199,7 +208,8 @@ static int __init load_uefi_certs(void)
if (!efi_enabled(EFI_SECURE_BOOT))
return 0;
 
-   mok = get_cert_list(L"MokListRT", _var, );
+   mok = get_cert_list(L"MokListRT", _var, ,
+   0, EFI_VARIABLE_NON_VOLATILE);
if (mok) {
rc = parse_efi_signature_list("UEFI:MokListRT",
  mok, moksize, get_handler_for_db);
@@ -208,7 +218,8 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
 
-   mokx = get_cert_list(L"MokListXRT", _var, );
+   mokx = get_cert_list(L"MokListXRT", _var, ,
+   0, EFI_VARIABLE_NON_VOLATILE);
if (mokx) {
rc = 

[PATCH 1/5] MODSIGN: do not load mok when secure boot disabled

[Lee, Chun-Yi]

The mok can not be trusted when the secure boot is disabled. Which
means that the kernel embedded certificate is the only trusted key.

Due to db/dbx are authenticated variables, they needs manufacturer's
KEK for update. So db/dbx are secure when secureboot disabled.

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: "Lee, Chun-Yi" 
---
 certs/load_uefi.c | 26 +++---
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index 3d88459..d6de4d0 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -164,17 +164,6 @@ static int __init load_uefi_certs(void)
}
}
 
-   mok = get_cert_list(L"MokListRT", _var, );
-   if (!mok) {
-   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
-   } else {
-   rc = parse_efi_signature_list("UEFI:MokListRT",
- mok, moksize, get_handler_for_db);
-   if (rc)
-   pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
-   kfree(mok);
-   }
-
dbx = get_cert_list(L"dbx", _var, );
if (!dbx) {
pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
@@ -187,6 +176,21 @@ static int __init load_uefi_certs(void)
kfree(dbx);
}
 
+   /* the MOK can not be trusted when secure boot is disabled */
+   if (!efi_enabled(EFI_SECURE_BOOT))
+   return 0;
+
+   mok = get_cert_list(L"MokListRT", _var, );
+   if (!mok) {
+   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
+   } else {
+   rc = parse_efi_signature_list("UEFI:MokListRT",
+ mok, moksize, get_handler_for_db);
+   if (rc)
+   pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
+   kfree(mok);
+   }
+
return rc;
 }
 late_initcall(load_uefi_certs);
-- 
2.10.2

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

[PATCH 2/5] MODSIGN: print appropriate status message when getting UEFI certificates list

[Lee, Chun-Yi]

When getting certificates list from UEFI variable, the original error
message shows the state number from UEFI firmware. It's hard to be read
by human. This patch changed the error message to show the appropriate
string.

The message will be showed as:

[0.788529] MODSIGN: Couldn't get UEFI MokListRT: EFI_NOT_FOUND
[0.788537] MODSIGN: Couldn't get UEFI MokListXRT: EFI_NOT_FOUND

Cc: David Howells 
Cc: Josh Boyer 
Cc: James Bottomley 
Signed-off-by: Lee, Chun-Yi 
---
 certs/load_uefi.c   | 43 ++-
 include/linux/efi.h | 25 +
 2 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index d6de4d0..f2f372b 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -4,6 +4,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "internal.h"
@@ -32,6 +33,24 @@ static __init bool uefi_check_ignore_db(void)
return status == EFI_SUCCESS;
 }
 
+static __init void print_get_fail(efi_char16_t *char16_str, efi_status_t 
status)
+{
+   char *utf8_str;
+   unsigned long utf8_size;
+
+   if (!char16_str)
+   return;
+   utf8_size = ucs2_utf8size(char16_str) + 1;
+   utf8_str = kmalloc(utf8_size, GFP_KERNEL);
+   if (!utf8_str)
+   return;
+   ucs2_as_utf8(utf8_str, char16_str, utf8_size);
+
+   pr_info("MODSIGN: Couldn't get UEFI %s: %s\n",
+   utf8_str, efi_status_to_str(status));
+   kfree(utf8_str);
+}
+
 /*
  * Get a certificate list blob from the named EFI variable.
  */
@@ -45,25 +64,29 @@ static __init void *get_cert_list(efi_char16_t *name, 
efi_guid_t *guid,
 
status = efi.get_variable(name, guid, NULL, , );
if (status != EFI_BUFFER_TOO_SMALL) {
-   pr_err("Couldn't get size: 0x%lx\n", status);
-   return NULL;
+   if (status != EFI_NOT_FOUND)
+   pr_err("Couldn't get size: 0x%lx\n", status);
+   goto err;
}
 
db = kmalloc(lsize, GFP_KERNEL);
if (!db) {
pr_err("Couldn't allocate memory for uefi cert list\n");
-   return NULL;
+   goto err;
}
 
status = efi.get_variable(name, guid, NULL, , db);
if (status != EFI_SUCCESS) {
kfree(db);
pr_err("Error reading db var: 0x%lx\n", status);
-   return NULL;
+   goto err;
}
 
*size = lsize;
return db;
+err:
+   print_get_fail(name, status);
+   return NULL;
 }
 
 /*
@@ -153,9 +176,7 @@ static int __init load_uefi_certs(void)
 */
if (!uefi_check_ignore_db()) {
db = get_cert_list(L"db", _var, );
-   if (!db) {
-   pr_err("MODSIGN: Couldn't get UEFI db list\n");
-   } else {
+   if (db) {
rc = parse_efi_signature_list("UEFI:db",
  db, dbsize, 
get_handler_for_db);
if (rc)
@@ -165,9 +186,7 @@ static int __init load_uefi_certs(void)
}
 
dbx = get_cert_list(L"dbx", _var, );
-   if (!dbx) {
-   pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
-   } else {
+   if (dbx) {
rc = parse_efi_signature_list("UEFI:dbx",
  dbx, dbxsize,
  get_handler_for_dbx);
@@ -181,9 +200,7 @@ static int __init load_uefi_certs(void)
return 0;
 
mok = get_cert_list(L"MokListRT", _var, );
-   if (!mok) {
-   pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
-   } else {
+   if (mok) {
rc = parse_efi_signature_list("UEFI:MokListRT",
  mok, moksize, get_handler_for_db);
if (rc)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2729d6f..c44946c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1600,4 +1600,29 @@ struct linux_efi_random_seed {
u8  bits[];
 };
 
+#define EFI_STATUS_STR(_status)\
+   case EFI_##_status: \
+   return "EFI_" __stringify(_status); \
+
+static inline char *
+efi_status_to_str(efi_status_t status)
+{
+   switch (status) {
+   EFI_STATUS_STR(SUCCESS)
+   EFI_STATUS_STR(LOAD_ERROR)
+   EFI_STATUS_STR(INVALID_PARAMETER)
+   EFI_STATUS_STR(UNSUPPORTED)
+   EFI_STATUS_STR(BAD_BUFFER_SIZE)
+   EFI_STATUS_STR(BUFFER_TOO_SMALL)
+   EFI_STATUS_STR(NOT_READY)
+   EFI_STATUS_STR(DEVICE_ERROR)
+   EFI_STATUS_STR(WRITE_PROTECTED)
+   EFI_STATUS_STR(OUT_OF_RESOURCES)
+   EFI_STATUS_STR(NOT_FOUND)
+   

[PATCH 0/5 v2] Using the hash in MOKx to blacklist kernel module

[Lee, Chun-Yi]

This patch set is base on the efi-lock-down and keys-uefi branchs in
David Howells's linux-fs git tree.

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-uefi

The main purpose is using the MOKx to blacklist kernel module.

As the MOK (Machine Owner Key), MOKx is a EFI boot time variable which
is maintained by shim boot loader. We can enroll the hash of blacklisted
kernel module (with or without signature) to MOKx by mokutil. Kernel loads
the hash from MOKx to blacklist keyring when booting. Kernel will prevent
to load the kernel module when its hash be found in blacklist.

This function is useful to revoke a kernel module that it has exploit. Or
revoking a kernel module that it was signed by a unsecure key.

Except MOKx, this patch set fixs another two issues: The MOK/MOKx should
not be loaded when secure boot is disabled. And, modified error message
prints out appropriate status string for reading by human being.

v2:
Chekcikng the attributes of db and mok before loading certificates.

Lee, Chun-Yi (5):
  MODSIGN: do not load mok when secure boot disabled
  MODSIGN: print appropriate status message when getting UEFI
certificates list
  MODSIGN: load blacklist from MOKx
  MODSIGN: checking the blacklisted hash before loading a kernel module
  MODSIGN: check the attributes of db and mok

 certs/load_uefi.c   | 92 +++--
 include/linux/efi.h | 25 ++
 kernel/module_signing.c | 62 +++--
 3 files changed, 152 insertions(+), 27 deletions(-)

-- 
2.10.2

--
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: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 10:23, Thiebaud Weksteen  wrote:
> On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel 
> wrote:
>
>> On 13 March 2018 at 07:47, Hans de Goede  wrote:
...
>> > Could the problem perhaps be that the new code for the TPM event-log is
>> > missing some handling to deal with running on a 32 bit firmware? I know
> the
>> > rest of the kernel has special code to deal with this.
>> >
>
> Yes, that was my guess as well.
>
>> That is a very good point, and I missed that this is a 64-bit kernel
>> running on 32-bit UEFI.
>
>> The TPM code does use efi_call_proto() directly, and now I am thinking
>> it is perhaps the allocate_pages() call that simply only initializes
>> the low 32-bits of log_tbl.
>
> That make sense. Would you know what happens to the arguments of the
> function in this case? (I'm thinking _location ?)

log_location and log_last_entry are always 64-bit quantities, and
efi_bool_t is always u8, so that shouldn't matter.

> Would it be safer to skip the code completely on EFI_MIXED systems?
>

Obviously, but I would like to avoid that if possible. Let's see first
if we've pinpointed it now.
--
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: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel 
wrote:

> On 13 March 2018 at 07:47, Hans de Goede  wrote:
> > Hi,
> >
> >
> > On 12-03-18 20:55, Thiebaud Weksteen wrote:
> >>
> ...
> >>
> >> Hans, you said you configured the tablet to use the 32-bit version of
grub
> >> instead
> >> of 64. Why's that?
> >
> >
> > Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
> > UEFI,
> > even though the processor is 64 bit capable (AFAIK 64 bit Windows
drivers
> > were
> > not ready in time so all Bay Trail devices shipped with a 32 bit
Windows).
> >
> > So this is running a 32 bit grub which boots a 64 bit kernel.
> >
> >> Jeremy, could you confirm if you are building the kernel in 64bit
mode? Is
> >> your Android install working? (that is, what happens if you boot
> >> Boot)?
> >
> >
> > AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64
bit.
> >
> > Could the problem perhaps be that the new code for the TPM event-log is
> > missing some handling to deal with running on a 32 bit firmware? I know
the
> > rest of the kernel has special code to deal with this.
> >

Yes, that was my guess as well.

> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.

> The TPM code does use efi_call_proto() directly, and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.

That make sense. Would you know what happens to the arguments of the
function in this case? (I'm thinking _location ?)
Would it be safer to skip the code completely on EFI_MIXED systems?

> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
--
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: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 12-03-18 22:02, Ard Biesheuvel wrote:

On 12 March 2018 at 19:55, Thiebaud Weksteen  wrote:

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:


On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:

On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <

ard.biesheu...@linaro.org>

wrote:


On 12 March 2018 at 17:01, Jeremy Cline  wrote:

On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:

On 12 March 2018 at 14:30, Jeremy Cline  wrote:

On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:

On 10 March 2018 at 10:45, Thiebaud Weksteen 

wrote:

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 

wrote:



On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen

wrote:

Thanks a lot for trying out the patch!

Please don't modify your install at this stage, I think we are

hitting a

firmware bug and that would be awesome if we can fix how we are

handling it.

So, if we reach that stage in the function it could either be

that:

* The allocation did not succeed, somehow, but the firmware

still

returned

EFI_SUCCEED.
* The size requested is incorrect (I'm thinking something like a

1G of

log). This would be due to either a miscalculation of log_size

(possible)

or; the returned values of GetEventLog are not correct.
I'm sending a patch to add checks for these. Could you please

apply and

retest?
Again, thanks for helping debugging this.



No problem, thanks for the help :)



With the new patch:



Locating the TCG2Protocol
Calling GetEventLog on TCG2Protocol
Log returned
log_location is not empty
log_size != 0
log_size < 1M
Allocating memory for storing the logs
Returned from memory allocation
Copying log to new location



And then it hangs. I added a couple more print statements:



diff --git a/drivers/firmware/efi/libstub/tpm.c

b/drivers/firmware/efi/libstub/tpm.c

index ee3fac109078..1ab5638bc50e 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -148,8 +148,11 @@ void

efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)

  efi_printk(sys_table_arg, "Copying log to new

location\n");



  memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
+   efi_printk(sys_table_arg, "Successfully memset log_tbl to

0\n");

  log_tbl->size = log_size;
+   efi_printk(sys_table_arg, "Set log_tbl->size\n");
  log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+   efi_printk(sys_table_arg, "Set log_tbl-version\n");
  memcpy(log_tbl->log, (void *) first_entry_addr,

log_size);



  efi_printk(sys_table_arg, "Installing the log into the

configuration table\n");


and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +

log_size);"


Thanks. Well, it looks like the memory that is supposedly

allocated

is not

usable. I'm thinking this is a firmware bug.
Ard, would you agree on this assumption? Thoughts on how to

proceed?




I am rather puzzled why the allocate_pool() should succeed and the
subsequent memset() should fail. This does not look like an issue

that

is intimately related to TPM2 support, rather an issue in the

firmware

that happens to get tickled after the change.

Would you mind trying replacing EFI_LOADER_DATA with
EFI_BOOT_SERVICES_DATA in the allocate_pool() call?


Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at

the

memset() call.



Could you try the following please? (attached as well in case gmail

mangles it)


diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index 2298560cea72..30d960a344b7 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -70,6 +70,8 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 size_t log_size, last_entry_size;
 efi_bool_t truncated;
 void *tcg2_protocol;
+   unsigned long num_pages;
+   efi_physical_addr_t log_tbl_alloc;

 status = efi_call_early(locate_protocol, _guid, NULL,
 _protocol);
@@ -104,9 +106,12 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 }

 /* Allocate space for the logs and copy them. */
-   status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-   sizeof(*log_tbl) + log_size,
-   (void **) _tbl);
+   num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,

EFI_PAGE_SIZE);

+   status = efi_call_early(allocate_pages,
+   EFI_ALLOCATE_ANY_PAGES,
+   EFI_LOADER_DATA,
+   num_pages,
+   _tbl_alloc);

 if (status != EFI_SUCCESS) {
 efi_printk(sys_table_arg,
@@ -114,6 +119,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 return;
 }

+   log_tbl = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 07:59, Ard Biesheuvel  wrote:
> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>> Hi,
>>
>>
>> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>>
> ...
>>>
>>> Hans, you said you configured the tablet to use the 32-bit version of grub
>>> instead
>>> of 64. Why's that?
>>
>>
>> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
>> UEFI,
>> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers
>> were
>> not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
>>
>> So this is running a 32 bit grub which boots a 64 bit kernel.
>>
>>> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
>>> your Android install working? (that is, what happens if you boot
>>> Boot)?
>>
>>
>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>
>> Could the problem perhaps be that the new code for the TPM event-log is
>> missing some handling to deal with running on a 32 bit firmware? I know the
>> rest of the kernel has special code to deal with this.
>>
>
> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.
>
> The TPM code does use efi_call_proto() directly,

*correctly*

> and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.
>
> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
--
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: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 07:47, Hans de Goede  wrote:
> Hi,
>
>
> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>
...
>>
>> Hans, you said you configured the tablet to use the 32-bit version of grub
>> instead
>> of 64. Why's that?
>
>
> Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit
> UEFI,
> even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers
> were
> not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
>
> So this is running a 32 bit grub which boots a 64 bit kernel.
>
>> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
>> your Android install working? (that is, what happens if you boot
>> Boot)?
>
>
> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>
> Could the problem perhaps be that the new code for the TPM event-log is
> missing some handling to deal with running on a 32 bit firmware? I know the
> rest of the kernel has special code to deal with this.
>

That is a very good point, and I missed that this is a 64-bit kernel
running on 32-bit UEFI.

The TPM code does use efi_call_proto() directly, and now I am thinking
it is perhaps the allocate_pages() call that simply only initializes
the low 32-bits of log_tbl.

Jeremy, could you please try initializing tcg2_protocol and log_tbl to
NULL at the start of the function?
--
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: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 12-03-18 20:55, Thiebaud Weksteen wrote:

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:


On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:

On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <

ard.biesheu...@linaro.org>

wrote:


On 12 March 2018 at 17:01, Jeremy Cline  wrote:

On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:

On 12 March 2018 at 14:30, Jeremy Cline  wrote:

On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:

On 10 March 2018 at 10:45, Thiebaud Weksteen 

wrote:

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 

wrote:



On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen

wrote:

Thanks a lot for trying out the patch!

Please don't modify your install at this stage, I think we are

hitting a

firmware bug and that would be awesome if we can fix how we are

handling it.

So, if we reach that stage in the function it could either be

that:

* The allocation did not succeed, somehow, but the firmware

still

returned

EFI_SUCCEED.
* The size requested is incorrect (I'm thinking something like a

1G of

log). This would be due to either a miscalculation of log_size

(possible)

or; the returned values of GetEventLog are not correct.
I'm sending a patch to add checks for these. Could you please

apply and

retest?
Again, thanks for helping debugging this.



No problem, thanks for the help :)



With the new patch:



Locating the TCG2Protocol
Calling GetEventLog on TCG2Protocol
Log returned
log_location is not empty
log_size != 0
log_size < 1M
Allocating memory for storing the logs
Returned from memory allocation
Copying log to new location



And then it hangs. I added a couple more print statements:



diff --git a/drivers/firmware/efi/libstub/tpm.c

b/drivers/firmware/efi/libstub/tpm.c

index ee3fac109078..1ab5638bc50e 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -148,8 +148,11 @@ void

efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)

  efi_printk(sys_table_arg, "Copying log to new

location\n");



  memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
+   efi_printk(sys_table_arg, "Successfully memset log_tbl to

0\n");

  log_tbl->size = log_size;
+   efi_printk(sys_table_arg, "Set log_tbl->size\n");
  log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+   efi_printk(sys_table_arg, "Set log_tbl-version\n");
  memcpy(log_tbl->log, (void *) first_entry_addr,

log_size);



  efi_printk(sys_table_arg, "Installing the log into the

configuration table\n");


and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +

log_size);"


Thanks. Well, it looks like the memory that is supposedly

allocated

is not

usable. I'm thinking this is a firmware bug.
Ard, would you agree on this assumption? Thoughts on how to

proceed?




I am rather puzzled why the allocate_pool() should succeed and the
subsequent memset() should fail. This does not look like an issue

that

is intimately related to TPM2 support, rather an issue in the

firmware

that happens to get tickled after the change.

Would you mind trying replacing EFI_LOADER_DATA with
EFI_BOOT_SERVICES_DATA in the allocate_pool() call?


Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at

the

memset() call.



Could you try the following please? (attached as well in case gmail

mangles it)


diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index 2298560cea72..30d960a344b7 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -70,6 +70,8 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 size_t log_size, last_entry_size;
 efi_bool_t truncated;
 void *tcg2_protocol;
+   unsigned long num_pages;
+   efi_physical_addr_t log_tbl_alloc;

 status = efi_call_early(locate_protocol, _guid, NULL,
 _protocol);
@@ -104,9 +106,12 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 }

 /* Allocate space for the logs and copy them. */
-   status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
-   sizeof(*log_tbl) + log_size,
-   (void **) _tbl);
+   num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,

EFI_PAGE_SIZE);

+   status = efi_call_early(allocate_pages,
+   EFI_ALLOCATE_ANY_PAGES,
+   EFI_LOADER_DATA,
+   num_pages,
+   _tbl_alloc);

 if (status != EFI_SUCCESS) {
 efi_printk(sys_table_arg,
@@ -114,6 +119,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
 return;
 }

+   log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned

long)log_tbl_alloc;

 

I pray you receive my mail in a good faith

[Mr.John Mark]

Good Day My Good Friend

Let me start by introducing myself I am Mr. John Mark from Burkina
Faso, I am writing you this letter based on latest development in my
bank which i we like to bring you in. The sum of Twelve Million Five
Hundred Thousand United State Dollars ($ 12.5Million) this is
legitimate Transition after the transfer we will share it, 50% for me
and 50% for you. Let me know if you Can you help me, kindly Contact me
for more details if you are interested in the deal

Contact me here...   john.mark79...@yahoo.com

Kind Regards

Mr. John Mark
--
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: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Mon, Mar 12, 2018 at 10:03 PM Ard Biesheuvel 
wrote:

> On 12 March 2018 at 19:55, Thiebaud Weksteen  wrote:
> > On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:
> >
> >> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> >> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> > ard.biesheu...@linaro.org>
> >> > wrote:
> >> >
> >> >> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> >> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >>  On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> >> > On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >> >> On 10 March 2018 at 10:45, Thiebaud Weksteen 
> >> > wrote:
> >> >>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
> >> > wrote:
> >> >>>
> >>  On Fri, Mar 09, 2018 at 10:43:50AM +, Thiebaud Weksteen
> > wrote:
> >> > Thanks a lot for trying out the patch!
> >> >
> >> > Please don't modify your install at this stage, I think we
are
> >> > hitting a
> >> > firmware bug and that would be awesome if we can fix how we
are
> >> >>> handling it.
> >> > So, if we reach that stage in the function it could either be
> >> > that:
> >> > * The allocation did not succeed, somehow, but the firmware
> > still
> >> >>> returned
> >> > EFI_SUCCEED.
> >> > * The size requested is incorrect (I'm thinking something
like a
> >> > 1G of
> >> > log). This would be due to either a miscalculation of
log_size
> >> >>> (possible)
> >> > or; the returned values of GetEventLog are not correct.
> >> > I'm sending a patch to add checks for these. Could you please
> >> > apply and
> >> > retest?
> >> > Again, thanks for helping debugging this.
> >> >>>
> >>  No problem, thanks for the help :)
> >> >>>
> >>  With the new patch:
> >> >>>
> >>  Locating the TCG2Protocol
> >>  Calling GetEventLog on TCG2Protocol
> >>  Log returned
> >>  log_location is not empty
> >>  log_size != 0
> >>  log_size < 1M
> >>  Allocating memory for storing the logs
> >>  Returned from memory allocation
> >>  Copying log to new location
> >> >>>
> >>  And then it hangs. I added a couple more print statements:
> >> >>>
> >>  diff --git a/drivers/firmware/efi/libstub/tpm.c
> >> >>> b/drivers/firmware/efi/libstub/tpm.c
> >>  index ee3fac109078..1ab5638bc50e 100644
> >>  --- a/drivers/firmware/efi/libstub/tpm.c
> >>  +++ b/drivers/firmware/efi/libstub/tpm.c
> >>  @@ -148,8 +148,11 @@ void
> >> >>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t
*sys_table_arg)
> >>   efi_printk(sys_table_arg, "Copying log to new
> >> > location\n");
> >> >>>
> >>   memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >>  +   efi_printk(sys_table_arg, "Successfully memset
log_tbl to
> >> > 0\n");
> >>   log_tbl->size = log_size;
> >>  +   efi_printk(sys_table_arg, "Set log_tbl->size\n");
> >>   log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >>  +   efi_printk(sys_table_arg, "Set log_tbl-version\n");
> >>   memcpy(log_tbl->log, (void *) first_entry_addr,
> > log_size);
> >> >>>
> >>   efi_printk(sys_table_arg, "Installing the log into
the
> >> >>> configuration table\n");
> >> >>>
> >>  and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
> >> > log_size);"
> >> >>>
> >> >>> Thanks. Well, it looks like the memory that is supposedly
> > allocated
> >> > is not
> >> >>> usable. I'm thinking this is a firmware bug.
> >> >>> Ard, would you agree on this assumption? Thoughts on how to
> > proceed?
> >> >>>
> >> >>
> >> >> I am rather puzzled why the allocate_pool() should succeed and
the
> >> >> subsequent memset() should fail. This does not look like an
issue
> >> > that
> >> >> is intimately related to TPM2 support, rather an issue in the
> >> > firmware
> >> >> that happens to get tickled after the change.
> >> >>
> >> >> Would you mind trying replacing EFI_LOADER_DATA with
> >> >> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
> >> >
> >> > Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still
hangs at
> >> > the
> >> > memset() call.
> >> >
> >> 
> >>  Could you try the following please? (attached as well in case
gmail
> >> > mangles it)
> >> 
> >>  diff --git a/drivers/firmware/efi/libstub/tpm.c
> >>  b/drivers/firmware/efi/libstub/tpm.c
> >>  index 2298560cea72..30d960a344b7 100644
> >>  --- a/drivers/firmware/efi/libstub/tpm.c
> >>  +++ b/drivers/firmware/efi/libstub/tpm.c
> >>  @@ -70,6 +70,8 @@ void
> >>