[PATCH v4 0/3] tpm: Don't propagate measurement failures to the verifiers layer

[PATCH v4 0/3] tpm: Don't propagate measurement failures to the verifiers layer

[Robbie Harwood]

Address review from Daniel (interdiff attached).  I moved the variable parsing
into its own function since it has gotten large enough I'd rather it not get
rewritten every time we need a variable as a boolean.

Note that I do not have a machine with a broken TPM for testing fallback.

Be well,
--Robbie

Robbie Harwood (3):
  types: make bool generally available
  env: add function for retrieving variables as booleans
  tpm: Don't propagate measurement failures to the verifiers layer

 docs/grub.texi                 | 10 ++++++++++
 grub-core/commands/parttool.c  |  2 +-
 grub-core/commands/tpm.c       | 21 +++++++++++++++++++--
 grub-core/kern/env.c           | 13 +++++++++++++
 grub-core/loader/arm64/linux.c |  1 -
 grub-core/parttool/msdospart.c |  4 ++--
 include/grub/env.h             |  1 +
 include/grub/parttool.h        |  2 +-
 include/grub/types.h           |  1 +
 9 files changed, 48 insertions(+), 7 deletions(-)

Interdiff against v3:
diff --git a/docs/grub.texi b/docs/grub.texi
index eb43d8970d..4ab5e9b588 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3829,8 +3829,9 @@ displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
 @node tpm_fail_fatal
 @subsection tpm_fail_fatal
 
-If this variable is enabled, TPM measurements that fail will be treated
-as fatal.  Otherwise, they will merely be debug-logged and boot will
+If this variable is set and true (i.e., not set to ``0'', ``false'',
+``disable'', or ``no''), TPM measurements that fail will be treated as
+fatal.  Otherwise, they will merely be debug-logged and boot will
 continue.
 
 
diff --git a/grub-core/commands/parttool.c b/grub-core/commands/parttool.c
index 051e31320e..ff45c65e61 100644
--- a/grub-core/commands/parttool.c
+++ b/grub-core/commands/parttool.c
@@ -315,7 +315,7 @@ grub_cmd_parttool (grub_command_t cmd __attribute__ ((unused)),
 		    switch (curarg->type)
 		      {
 		      case GRUB_PARTTOOL_ARG_BOOL:
-			pargs[curarg - ptool->args].bool
+			pargs[curarg - ptool->args].b
 			  = (args[j][grub_strlen (curarg->name)] != '-');
 			break;
 
diff --git a/grub-core/commands/tpm.c b/grub-core/commands/tpm.c
index ca088055dd..3437e8e03b 100644
--- a/grub-core/commands/tpm.c
+++ b/grub-core/commands/tpm.c
@@ -27,7 +27,6 @@
 #include <grub/term.h>
 #include <grub/verify.h>
 #include <grub/dl.h>
-#include <stdbool.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -44,12 +43,7 @@ grub_tpm_verify_init (grub_file_t io,
 static inline bool
 is_tpm_fail_fatal (void)
 {
-  const char *val = grub_env_get ("tpm_fail_fatal");
-
-  if (val == NULL || grub_strlen (val) < 1 || grub_strcmp (val, "0") == 0 ||
-      grub_strcmp (val, "false") == 0)
-    return false;
-  return true;
+  return grub_env_get_bool ("tpm_fail_fatal", false);
 }
 
 static grub_err_t
@@ -85,7 +79,7 @@ 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_ERR_NONE;
+    return grub_errno;
   grub_memcpy (description, prefix, grub_strlen (prefix));
   grub_memcpy (description + grub_strlen (prefix), str,
 	       grub_strlen (str) + 1);
diff --git a/grub-core/kern/env.c b/grub-core/kern/env.c
index 10e08ad76c..7640688963 100644
--- a/grub-core/kern/env.c
+++ b/grub-core/kern/env.c
@@ -144,6 +144,19 @@ grub_env_get (const char *name)
   return var->value;
 }
 
+bool
+grub_env_get_bool (const char *name, bool if_unset)
+{
+  const char *val = grub_env_get (name);
+
+  if (val == NULL || grub_strlen (val) < 1)
+    return if_unset;
+  if (grub_strcmp (val, "0") == 0 || grub_strcmp (val, "false") == 0 ||
+      grub_strcmp (val, "disable") == 0 || grub_strcmp (val, "no") == 0)
+    return false;
+  return true;
+}
+
 void
 grub_env_unset (const char *name)
 {
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 9d0bacc854..48ab34a256 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -33,7 +33,6 @@
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
 #include <grub/verify.h>
-#include <stdbool.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
diff --git a/grub-core/parttool/msdospart.c b/grub-core/parttool/msdospart.c
index 3918caa06e..3a7699e454 100644
--- a/grub-core/parttool/msdospart.c
+++ b/grub-core/parttool/msdospart.c
@@ -61,7 +61,7 @@ static grub_err_t grub_pcpart_boot (const grub_device_t dev,
       return grub_errno;
     }
 
-  if (args[0].set && args[0].bool)
+  if (args[0].set && args[0].b)
     {
       for (i = 0; i < 4; i++)
 	mbr.entries[i].flag = 0x0;
@@ -116,7 +116,7 @@ static grub_err_t grub_pcpart_type (const grub_device_t dev,
 
   if (args[1].set)
     {
-      if (args[1].bool)
+      if (args[1].b)
 	type |= GRUB_PC_PARTITION_TYPE_HIDDEN_FLAG;
       else
 	type &= ~GRUB_PC_PARTITION_TYPE_HIDDEN_FLAG;
diff --git a/include/grub/env.h b/include/grub/env.h
index 76f832eb94..6b9379a300 100644
--- a/include/grub/env.h
+++ b/include/grub/env.h
@@ -45,6 +45,7 @@ struct grub_env_var
 
 grub_err_t EXPORT_FUNC(grub_env_set) (const char *name, const char *val);
 const char *EXPORT_FUNC(grub_env_get) (const char *name);
+bool EXPORT_FUNC(grub_env_get_bool) (const char *name, bool if_unset);
 void EXPORT_FUNC(grub_env_unset) (const char *name);
 struct grub_env_var *EXPORT_FUNC(grub_env_update_get_sorted) (void);
 
diff --git a/include/grub/parttool.h b/include/grub/parttool.h
index 4e8f8d5e51..4799a22c5d 100644
--- a/include/grub/parttool.h
+++ b/include/grub/parttool.h
@@ -32,7 +32,7 @@ struct grub_parttool_args
   int set;
   union
   {
-    int bool;
+    int b;
     char *str;
   };
 };
diff --git a/include/grub/types.h b/include/grub/types.h
index 5ae0ced388..6d5dc5cdaa 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -20,6 +20,7 @@
 #define GRUB_TYPES_HEADER	1
 
 #include <config.h>
+#include <stdbool.h>
 #ifndef GRUB_UTIL
 #include <grub/cpu/types.h>
 #endif
-- 
2.35.1

[PATCH v4 1/3] types: make bool generally available

[Robbie Harwood]

Add an include on stdbool.h, making the bool type generally available
within grub without needing to add a file-specific include every time it
would be used.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/commands/parttool.c  | 2 +-
 grub-core/loader/arm64/linux.c | 1 -
 grub-core/parttool/msdospart.c | 4 ++--
 include/grub/parttool.h        | 2 +-
 include/grub/types.h           | 1 +
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/grub-core/commands/parttool.c b/grub-core/commands/parttool.c
index 051e31320e..ff45c65e61 100644
--- a/grub-core/commands/parttool.c
+++ b/grub-core/commands/parttool.c
@@ -315,7 +315,7 @@ grub_cmd_parttool (grub_command_t cmd __attribute__ ((unused)),
 		    switch (curarg->type)
 		      {
 		      case GRUB_PARTTOOL_ARG_BOOL:
-			pargs[curarg - ptool->args].bool
+			pargs[curarg - ptool->args].b
 			  = (args[j][grub_strlen (curarg->name)] != '-');
 			break;
 
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index 9d0bacc854..48ab34a256 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -33,7 +33,6 @@
 #include <grub/i18n.h>
 #include <grub/lib/cmdline.h>
 #include <grub/verify.h>
-#include <stdbool.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
diff --git a/grub-core/parttool/msdospart.c b/grub-core/parttool/msdospart.c
index 3918caa06e..3a7699e454 100644
--- a/grub-core/parttool/msdospart.c
+++ b/grub-core/parttool/msdospart.c
@@ -61,7 +61,7 @@ static grub_err_t grub_pcpart_boot (const grub_device_t dev,
       return grub_errno;
     }
 
-  if (args[0].set && args[0].bool)
+  if (args[0].set && args[0].b)
     {
       for (i = 0; i < 4; i++)
 	mbr.entries[i].flag = 0x0;
@@ -116,7 +116,7 @@ static grub_err_t grub_pcpart_type (const grub_device_t dev,
 
   if (args[1].set)
     {
-      if (args[1].bool)
+      if (args[1].b)
 	type |= GRUB_PC_PARTITION_TYPE_HIDDEN_FLAG;
       else
 	type &= ~GRUB_PC_PARTITION_TYPE_HIDDEN_FLAG;
diff --git a/include/grub/parttool.h b/include/grub/parttool.h
index 4e8f8d5e51..4799a22c5d 100644
--- a/include/grub/parttool.h
+++ b/include/grub/parttool.h
@@ -32,7 +32,7 @@ struct grub_parttool_args
   int set;
   union
   {
-    int bool;
+    int b;
     char *str;
   };
 };
diff --git a/include/grub/types.h b/include/grub/types.h
index 5ae0ced388..6d5dc5cdaa 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -20,6 +20,7 @@
 #define GRUB_TYPES_HEADER	1
 
 #include <config.h>
+#include <stdbool.h>
 #ifndef GRUB_UTIL
 #include <grub/cpu/types.h>
 #endif
-- 
2.35.1

[PATCH v4 2/3] env: add function for retrieving variables as booleans

[Robbie Harwood]

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 grub-core/kern/env.c | 13 +++++++++++++
 include/grub/env.h   |  1 +
 2 files changed, 14 insertions(+)

diff --git a/grub-core/kern/env.c b/grub-core/kern/env.c
index 10e08ad76c..7640688963 100644
--- a/grub-core/kern/env.c
+++ b/grub-core/kern/env.c
@@ -144,6 +144,19 @@ grub_env_get (const char *name)
   return var->value;
 }
 
+bool
+grub_env_get_bool (const char *name, bool if_unset)
+{
+  const char *val = grub_env_get (name);
+
+  if (val == NULL || grub_strlen (val) < 1)
+    return if_unset;
+  if (grub_strcmp (val, "0") == 0 || grub_strcmp (val, "false") == 0 ||
+      grub_strcmp (val, "disable") == 0 || grub_strcmp (val, "no") == 0)
+    return false;
+  return true;
+}
+
 void
 grub_env_unset (const char *name)
 {
diff --git a/include/grub/env.h b/include/grub/env.h
index 76f832eb94..6b9379a300 100644
--- a/include/grub/env.h
+++ b/include/grub/env.h
@@ -45,6 +45,7 @@ struct grub_env_var
 
 grub_err_t EXPORT_FUNC(grub_env_set) (const char *name, const char *val);
 const char *EXPORT_FUNC(grub_env_get) (const char *name);
+bool EXPORT_FUNC(grub_env_get_bool) (const char *name, bool if_unset);
 void EXPORT_FUNC(grub_env_unset) (const char *name);
 struct grub_env_var *EXPORT_FUNC(grub_env_update_get_sorted) (void);
 
-- 
2.35.1

[PATCH v4 3/3] tpm: Don't propagate measurement failures to the verifiers layer

[Robbie Harwood]

Currently if an EFI firmware fails to do a TPM measurement for a file,
the error will be propagated to the verifiers framework which will
prevent it to be opened.  This mean that buggy firmwares will lead to
the system not booting because files won't be allowed to be loaded. But
a failure to do a TPM measurement isn't expected to be a fatal error
that causes the system to be unbootable.

To avoid this, don't return errors from .write and .verify_string
callbacks and just print a debug message in the case of a TPM
measurement failure.  Add an environment variable (tpm_fail_fatal) to
restore the previous behavior.

Also-authored-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 docs/grub.texi           | 10 ++++++++++
 grub-core/commands/tpm.c | 21 +++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 2d6cd83580..4ab5e9b588 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3318,6 +3318,7 @@ These variables have special meaning to GRUB.
 * theme::
 * timeout::
 * timeout_style::
+* tpm_fail_fatal::
 @end menu
 
 
@@ -3825,6 +3826,15 @@ displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
 (@pxref{Simple configuration}) for details.
 
 
+@node tpm_fail_fatal
+@subsection tpm_fail_fatal
+
+If this variable is set and true (i.e., not set to ``0'', ``false'',
+``disable'', or ``no''), TPM measurements that fail will be treated as
+fatal.  Otherwise, they will merely be debug-logged and boot will
+continue.
+
+
 @node Environment block
 @section The GRUB environment block
 
diff --git a/grub-core/commands/tpm.c b/grub-core/commands/tpm.c
index 2052c36eab..3437e8e03b 100644
--- a/grub-core/commands/tpm.c
+++ b/grub-core/commands/tpm.c
@@ -18,6 +18,7 @@
  *  Core TPM support code.
  */
 
+#include <grub/env.h>
 #include <grub/err.h>
 #include <grub/i18n.h>
 #include <grub/misc.h>
@@ -39,10 +40,22 @@ grub_tpm_verify_init (grub_file_t io,
   return GRUB_ERR_NONE;
 }
 
+static inline bool
+is_tpm_fail_fatal (void)
+{
+  return grub_env_get_bool ("tpm_fail_fatal", false);
+}
+
 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_err_t status = grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
+
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring buffer failed: %d\n", status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -74,7 +87,11 @@ grub_tpm_verify_string (char *str, enum grub_verify_string_type type)
     grub_tpm_measure ((unsigned char *) str, grub_strlen (str),
 		      GRUB_STRING_PCR, description);
   grub_free (description);
-  return status;
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring string %s failed: %d\n", str, status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 struct grub_file_verifier grub_tpm_verifier = {
-- 
2.35.1

Re: [PATCH v4 0/3] tpm: Don't propagate measurement failures to the verifiers layer

[Daniel Kiper]

On Fri, Nov 04, 2022 at 12:13:33PM -0400, Robbie Harwood wrote:
> Address review from Daniel (interdiff attached).  I moved the variable parsing
> into its own function since it has gotten large enough I'd rather it not get
> rewritten every time we need a variable as a boolean.
>
> Note that I do not have a machine with a broken TPM for testing fallback.
>
> Be well,
> --Robbie
>
> Robbie Harwood (3):
>   types: make bool generally available
>   env: add function for retrieving variables as booleans
>   tpm: Don't propagate measurement failures to the verifiers layer

For all patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thank you for adding these two "bool" patches!

Daniel