Re: [PATCH v3 1/1] tpm: Don't propagate measurement failures to the verifiers layer

[Daniel Kiper]

On Mon, Oct 31, 2022 at 05:31:40PM -0400, Robbie Harwood wrote:
> 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           |  9 +++++++++
>  grub-core/commands/tpm.c | 29 ++++++++++++++++++++++++++---
>  2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 2d6cd83580..eb43d8970d 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,14 @@ 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 enabled, TPM measurements that fail will be treated

What do you mean by enabled? Please align description in doc with the
code below.

> +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..ca088055dd 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>
> @@ -26,6 +27,7 @@
>  #include <grub/term.h>
>  #include <grub/verify.h>
>  #include <grub/dl.h>
> +#include <stdbool.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -39,10 +41,27 @@ grub_tpm_verify_init (grub_file_t io,
>    return GRUB_ERR_NONE;
>  }
>
> +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)

I would add "disable" and "no" to the list.

> +    return false;
> +  return true;
> +}
> +
>  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
> @@ -66,7 +85,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_errno;
> +    return GRUB_ERR_NONE;

This change seems wrong to me. I think it should be dropped.

Daniel