Re: [PATCH] tpm: Pass unknown error as non-fatal, but debug print the error we got

[Javier Martinez Canillas]

Hello Mathieu,

On 10/25/19 4:48 PM, Mathieu Trudel-Lapierre wrote:
> On Fri, Oct 25, 2019 at 10:28 AM Mathieu Trudel-Lapierre
> <address@hidden> wrote:
>>
>> Signed-off-by: Mathieu Trudel-Lapierre <address@hidden>
>> Patch-Name: ubuntu-tpm-unknown-error-non-fatal.patch
>> ---
>>  grub-core/commands/efi/tpm.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
> 
> I see I omitted to explain why I'm proposing this.
> 
> I've seen a couple of reports so far of issues with booting with TPM
> measurement enabled, when the firmware has TPM enabled, on some
> hardware.
> 
> In particular, this has happened on a Dell laptop at Plumbers this
> year (an older model XPS15 IIRC), and a few different models of
> laptops/motherboards. Some report having a TPM, and some do not:
> 
> HP EliteBook 820 G4 (Infineon SLB9670?)
> ASUS M32CD4-K motherboard (unknown)
> ASUS ROG GL553VE Laptop (unknown)
> ASUS ZenBook 3 UX390UA (unknown)
> ASUS Zenbook UX305FA (unspecified TPM)
> ASUS ZenBook UX303UA (unknown)
> ASUS 2O7HSV6 ??
> 
> See https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/1848892.
>

Yes, we also got similar reports for Fedora, i.e:

https://bugzilla.redhat.com/show_bug.cgi?id=1645903
 
> Unfortunately the reports are not of great quality, but I'm starting
> to worry about what exactly is wrong, if it's really a firmware / TPM
> issue or a bug in the TPM code.
> 
> For now, it seems like the best is to get more information as to what
> exactly the failure is (hence grub_dprintf()), and treating these

Agreed. It would be good to get know the exact EFI status code returned by
the firmware in the case of a failure.

> errors as non-fatal so people can still boot.
>

I think that we should go even further and make all the TPM measurement
errors to be non-fatal. For example something like the following patch [0].

> After briefly discussing this with others, it's not clear whether all
> the affected systems really do have a TPM, but they might still report
> in firmware that they do. Are we running into a case where the
> firmware wrongly reports there is a TPM, but fails to do any
> measurements?
> 

That's interesting. I see that EFI_TCG2_PROTOCOL.GetCapability() is called
and the EFI_TCG2_BOOT_SERVICE_CAPABILITY.TPMPresentFlag checked to know if
a TPM is present or not. So would be very weird that the firmware reported
that a TPM is present but that's not the case.

Maybe the machines did have a TPM but the reporter just didn't know?

[0]:
>From 0d404b65cddcf92e96d3cfa6e23b82e336d2535b Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <address@hidden>
Date: Fri, 25 Oct 2019 19:27:26 +0200
Subject: [RFC PATCH] tpm: Don't propagate TPM measurement failures to the
 verifiers layer

Currently if the EFI firmware fails to do a TPM measurement for a file,
the error will be propagated to the verifiers framework and so opening
the file will not succeed.

This mean that buggy firmwares will prevent an operating system to boot
since the loader won't be able to open the kernel binaries. But failing
to do TPM measurements shouldn't be a fatal error and the system should
still be able to boot.

Signed-off-by: Javier Martinez Canillas <address@hidden>
---
 grub-core/commands/tpm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git grub-core/commands/tpm.c grub-core/commands/tpm.c
index 1441c494d81..dbaeae46dfa 100644
--- grub-core/commands/tpm.c
+++ grub-core/commands/tpm.c
@@ -49,7 +49,8 @@ grub_tpm_verify_init (grub_file_t io,
 static grub_err_t
 grub_tpm_verify_write (void *context, void *buf, grub_size_t size)
 {
-  return grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
+  grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -57,7 +58,6 @@ grub_tpm_verify_string (char *str, enum 
grub_verify_string_type type)
 {
   const char *prefix = NULL;
   char *description;
-  grub_err_t status;
 
   switch (type)
     {
@@ -73,15 +73,15 @@ grub_tpm_verify_string (char *str, enum 
grub_verify_string_type type)
     }
   description = grub_malloc (grub_strlen (str) + grub_strlen (prefix) + 1);
   if (!description)
-    return grub_errno;
+    return GRUB_ERR_NONE;
   grub_memcpy (description, prefix, grub_strlen (prefix));
   grub_memcpy (description + grub_strlen (prefix), str,
               grub_strlen (str) + 1);
-  status =
-    grub_tpm_measure ((unsigned char *) str, grub_strlen (str),
-                     GRUB_STRING_PCR, description);
+
+  grub_tpm_measure ((unsigned char *) str, grub_strlen (str), GRUB_STRING_PCR,
+                    description);
   grub_free (description);
-  return status;
+  return GRUB_ERR_NONE;
 }
 
 struct grub_file_verifier grub_tpm_verifier = {
-- 
2.21.0

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat