All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] Enable TDX measurement to RTMR register
@ 2022-03-14  6:52 Lu Ken
  2022-04-27 14:36 ` Daniel Kiper
  0 siblings, 1 reply; 5+ messages in thread
From: Lu Ken @ 2022-03-14  6:52 UTC (permalink / raw)
  To: grub-devel; +Cc: min.m.xu, Lu Ken

Intel Trust Domain Extensions(Intel TDX) refers to an Intel technology
that extends Virtual Machine Extensions(VMX) and Multi-Key Total Memory
Encryption(MK-TME) with a new kind of virtual machine guest called a
Trust Domain(TD)[1]. A TD runs in a CPU mode that protects the confidentiality
of its memory contents and its CPU state from any other software, including
the hosting Virtual Machine Monitor (VMM).

Trust Domain Virtual Firmware (TDVF) is required to provide TD services to
the TD guest OS.[2] Its reference code is available at https://github.com/tianocore/edk2-staging/tree/TDVF.

To support TD measurement/attestation, TDs provide 4 RTMR registers like
TPM/TPM2 PCR as below:
- RTMR[0] is for TDVF configuration
- RTMR[1] is for the TD OS loader and kernel
- RTMR[2] is for the OS application
- RTMR[3] is reserved for special usage only

This patch adds TD Measurement protocol support along with TPM/TPM2 protocol.

References:
[1] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf
[2] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf

Signed-off-by: Lu Ken <ken.lu@intel.com>
---

Notes:
    v2 changes:
    - Separate the CC_MEASUREMENT code to standalone file grub-core/commands/efi/cc.c
    - Use NULL explicity for the judgement like "if (cc != NULL)"
    - Include all headers for all types, structure used in cc.h
    - Add the prefix "GRUB_"for all macro definition and the prefix "grub_" for type or structure's field
    - Align one argument in one line

 grub-core/Makefile.core.def  |   1 +
 grub-core/commands/efi/cc.c  |  88 +++++++++++++++++++
 grub-core/commands/efi/tpm.c |   3 +
 include/grub/efi/cc.h        | 164 +++++++++++++++++++++++++++++++++++
 4 files changed, 256 insertions(+)
 create mode 100644 grub-core/commands/efi/cc.c
 create mode 100644 include/grub/efi/cc.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 6b00eb555..dbae796a7 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -2561,6 +2561,7 @@ module = {
   name = tpm;
   common = commands/tpm.c;
   efi = commands/efi/tpm.c;
+  efi = commands/efi/cc.c;
   enable = efi;
 };
 
diff --git a/grub-core/commands/efi/cc.c b/grub-core/commands/efi/cc.c
new file mode 100644
index 000000000..0e6b417c4
--- /dev/null
+++ b/grub-core/commands/efi/cc.c
@@ -0,0 +1,88 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB 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 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ *  EFI CC measurement support code.
+ */
+
+#include <grub/err.h>
+#include <grub/i18n.h>
+#include <grub/efi/api.h>
+#include <grub/efi/efi.h>
+#include <grub/efi/cc.h>
+#include <grub/tpm.h>
+#include <grub/mm.h>
+
+static grub_efi_guid_t cc_measurement_guid = GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID;
+
+static inline grub_err_t
+grub_efi_log_event_status (grub_efi_status_t status)
+{
+  switch (status)
+    {
+    case GRUB_EFI_SUCCESS:
+      return 0;
+    case GRUB_EFI_DEVICE_ERROR:
+      return grub_error (GRUB_ERR_IO, N_("Command failed"));
+    case GRUB_EFI_INVALID_PARAMETER:
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Invalid parameter"));
+    case GRUB_EFI_BUFFER_TOO_SMALL:
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too small"));
+    case GRUB_EFI_NOT_FOUND:
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("TPM unavailable"));
+    default:
+      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("Unknown TPM error"));
+    }
+}
+
+grub_err_t
+grub_cc_log_event (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
+		   const char *description)
+{
+  grub_efi_cc_event_t *event;
+  grub_efi_status_t status;
+  grub_efi_cc_protocol_t *cc;
+  grub_efi_cc_mr_index_t mr;
+
+  cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL);
+  if (cc == NULL)
+    return 0;
+
+  status = efi_call_3 (cc->map_pcr_to_mr_index, cc, pcr, &mr);
+  if (status != GRUB_EFI_SUCCESS)
+    return grub_efi_log_event_status (status);
+
+  event = grub_zalloc (sizeof (grub_efi_cc_event_t)
+		       + grub_strlen (description) + 1);
+  if (event == NULL)
+    return grub_error (GRUB_ERR_OUT_OF_MEMORY,
+		       N_ ("cannot allocate CC event buffer"));
+
+  event->Header.HeaderSize = sizeof (grub_efi_cc_event_header_t);
+  event->Header.HeaderVersion = GRUB_EFI_CC_EVENT_HEADER_VERSION;
+  event->Header.MrIndex = mr;
+  event->Header.EventType = EV_IPL;
+  event->Size = sizeof (*event) - sizeof (event->Event)
+		+ grub_strlen (description) + 1;
+  grub_memcpy (event->Event, description, grub_strlen (description) + 1);
+
+  status = efi_call_5 (cc->hash_log_extend_event, cc, 0,
+		       (grub_efi_physical_address_t) buf,
+		       (grub_efi_uint64_t) size, event);
+  grub_free (event);
+
+  return grub_efi_log_event_status (status);
+}
diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
index a97d85368..c40093fb4 100644
--- a/grub-core/commands/efi/tpm.c
+++ b/grub-core/commands/efi/tpm.c
@@ -22,6 +22,7 @@
 #include <grub/i18n.h>
 #include <grub/efi/api.h>
 #include <grub/efi/efi.h>
+#include <grub/efi/cc.h>
 #include <grub/efi/tpm.h>
 #include <grub/mm.h>
 #include <grub/tpm.h>
@@ -228,6 +229,8 @@ grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
   grub_efi_handle_t tpm_handle;
   grub_efi_uint8_t protocol_version;
 
+  grub_cc_log_event(buf, size, pcr, description);
+
   if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
     return 0;
 
diff --git a/include/grub/efi/cc.h b/include/grub/efi/cc.h
new file mode 100644
index 000000000..6c9ea52e6
--- /dev/null
+++ b/include/grub/efi/cc.h
@@ -0,0 +1,164 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2022  Free Software Foundation, Inc.
+ *
+ *  GRUB 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 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_EFI_CC_HEADER
+#define GRUB_EFI_CC_HEADER 1
+
+#include <grub/efi/api.h>
+#include <grub/efi/efi.h>
+#include <grub/err.h>
+
+#define GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID \
+  { 0x96751a3d, 0x72f4, 0x41a6, \
+    { 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b } \
+  };
+
+struct grub_efi_cc_version
+{
+  grub_efi_uint8_t Major;
+  grub_efi_uint8_t Minor;
+};
+typedef struct grub_efi_cc_version grub_efi_cc_version_t;
+
+/*
+ * EFI_CC Type/SubType definition
+ */
+#define GRUB_EFI_CC_TYPE_NONE	0
+#define GRUB_EFI_CC_TYPE_SEV	1
+#define GRUB_EFI_CC_TYPE_TDX	2
+
+struct grub_efi_cc_type
+{
+  grub_efi_uint8_t Type;
+  grub_efi_uint8_t SubType;
+};
+typedef struct grub_efi_cc_type grub_efi_cc_type_t;
+
+typedef grub_efi_uint32_t grub_efi_cc_event_log_bitmap_t;
+typedef grub_efi_uint32_t grub_efi_cc_event_log_format_t;
+typedef grub_efi_uint32_t grub_efi_cc_event_algorithm_bitmap_t;
+typedef grub_efi_uint32_t grub_efi_cc_mr_index_t;
+
+/*
+ * Intel TDX measure register index
+ */
+#define GRUB_TDX_MR_INDEX_MRTD	0
+#define GRUB_TDX_MR_INDEX_RTMR0	1
+#define GRUB_TDX_MR_INDEX_RTMR1	2
+#define GRUB_TDX_MR_INDEX_RTMR2	3
+#define GRUB_TDX_MR_INDEX_RTMR3	4
+
+#define GRUB_EFI_CC_EVENT_LOG_FORMAT_TCG_2	0x00000002
+#define GRUB_EFI_CC_BOOT_HASH_ALG_SHA384	0x00000004
+#define GRUB_EFI_CC_EVENT_HEADER_VERSION	1
+
+struct grub_efi_cc_event_header
+{
+  /*
+   * Size of the event header itself (sizeof(EFI_TD_EVENT_HEADER))
+   */
+  grub_efi_uint32_t      HeaderSize;
+
+  /*
+   * Header version. For this version of this specification,
+   * the value shall be 1.
+   */
+  grub_efi_uint16_t      HeaderVersion;
+
+  /*
+   * Index of the MR that shall be extended
+   */
+  grub_efi_cc_mr_index_t MrIndex;
+
+  /*
+   * Type of the event that shall be extended (and optionally logged)
+   */
+  grub_efi_uint32_t      EventType;
+} GRUB_PACKED;
+typedef struct grub_efi_cc_event_header grub_efi_cc_event_header_t;
+
+struct grub_efi_cc_event
+{
+  /*
+   * Total size of the event including the Size component, the header and the
+   * Event data.
+   */
+  grub_efi_uint32_t          Size;
+  grub_efi_cc_event_header_t Header;
+  grub_efi_uint8_t           Event[1];
+} GRUB_PACKED;
+typedef struct grub_efi_cc_event grub_efi_cc_event_t;
+
+struct grub_efi_cc_boot_service_capability
+{
+  /* Allocated size of the structure */
+  grub_efi_uint8_t                     Size;
+
+  /*
+   * Version of the grub_efi_cc_boot_service_capability_t structure itself.
+   * For this version of the protocol, the Major version shall be set to 1
+   * and the Minor version shall be set to 1.
+   */
+  grub_efi_cc_version_t                StructureVersion;
+
+  /*
+   * Version of the EFI TD protocol.
+   * For this version of the protocol, the Major version shall be set to 1
+   * and the Minor version shall be set to 1.
+   */
+  grub_efi_cc_version_t                ProtocolVersion;
+
+  /* Supported hash algorithms */
+  grub_efi_cc_event_algorithm_bitmap_t HashAlgorithmBitmap;
+
+  /* Bitmap of supported event log formats */
+  grub_efi_cc_event_log_bitmap_t       SupportedEventLogs;
+
+  /* Indicates the CC type */
+  grub_efi_cc_type_t CcType;
+};
+typedef struct grub_efi_cc_boot_service_capability grub_efi_cc_boot_service_capability_t;
+
+struct grub_efi_cc_protocol
+{
+  grub_efi_status_t (*get_capability) (
+	  struct grub_efi_cc_protocol *this,
+	  grub_efi_cc_boot_service_capability_t *ProtocolCapability);
+  grub_efi_status_t (*get_event_log) (
+	  struct grub_efi_cc_protocol *this,
+	  grub_efi_cc_event_log_format_t EventLogFormat,
+	  grub_efi_physical_address_t *EventLogLocation,
+	  grub_efi_physical_address_t *EventLogLastEntry,
+	  grub_efi_boolean_t *EventLogTruncated);
+  grub_efi_status_t (*hash_log_extend_event) (
+	  struct grub_efi_cc_protocol *this,
+	  grub_efi_uint64_t Flags,
+	  grub_efi_physical_address_t DataToHash,
+	  grub_efi_uint64_t DataToHashLen,
+	  grub_efi_cc_event_t *EfiCcEvent);
+  grub_efi_status_t (*map_pcr_to_mr_index) (
+	  struct grub_efi_cc_protocol *this,
+	  grub_efi_uint32_t PcrIndex,
+	  grub_efi_cc_mr_index_t *MrIndex);
+};
+typedef struct grub_efi_cc_protocol grub_efi_cc_protocol_t;
+
+grub_err_t grub_cc_log_event (unsigned char *buf, grub_size_t size,
+			      grub_uint8_t pcr, const char *description);
+
+#endif
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] Enable TDX measurement to RTMR register
  2022-03-14  6:52 [PATCH V2] Enable TDX measurement to RTMR register Lu Ken
@ 2022-04-27 14:36 ` Daniel Kiper
  2022-04-29  4:25   ` Lu, Ken
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Kiper @ 2022-04-27 14:36 UTC (permalink / raw)
  To: Lu Ken; +Cc: grub-devel, min.m.xu

First of all, sorry for late reply but I am busy...

On Mon, Mar 14, 2022 at 02:52:22PM +0800, Lu Ken wrote:
> Intel Trust Domain Extensions(Intel TDX) refers to an Intel technology
> that extends Virtual Machine Extensions(VMX) and Multi-Key Total Memory
> Encryption(MK-TME) with a new kind of virtual machine guest called a
> Trust Domain(TD)[1]. A TD runs in a CPU mode that protects the confidentiality
> of its memory contents and its CPU state from any other software, including
> the hosting Virtual Machine Monitor (VMM).
>
> Trust Domain Virtual Firmware (TDVF) is required to provide TD services to
> the TD guest OS.[2] Its reference code is available at https://github.com/tianocore/edk2-staging/tree/TDVF.
>
> To support TD measurement/attestation, TDs provide 4 RTMR registers like
> TPM/TPM2 PCR as below:
> - RTMR[0] is for TDVF configuration
> - RTMR[1] is for the TD OS loader and kernel
> - RTMR[2] is for the OS application
> - RTMR[3] is reserved for special usage only
>
> This patch adds TD Measurement protocol support along with TPM/TPM2 protocol.
>
> References:
> [1] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf
> [2] https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf
>
> Signed-off-by: Lu Ken <ken.lu@intel.com>
> ---
>
> Notes:
>     v2 changes:
>     - Separate the CC_MEASUREMENT code to standalone file grub-core/commands/efi/cc.c
>     - Use NULL explicity for the judgement like "if (cc != NULL)"
>     - Include all headers for all types, structure used in cc.h
>     - Add the prefix "GRUB_"for all macro definition and the prefix "grub_" for type or structure's field
>     - Align one argument in one line
>
>  grub-core/Makefile.core.def  |   1 +
>  grub-core/commands/efi/cc.c  |  88 +++++++++++++++++++
>  grub-core/commands/efi/tpm.c |   3 +
>  include/grub/efi/cc.h        | 164 +++++++++++++++++++++++++++++++++++
>  4 files changed, 256 insertions(+)
>  create mode 100644 grub-core/commands/efi/cc.c
>  create mode 100644 include/grub/efi/cc.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 6b00eb555..dbae796a7 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -2561,6 +2561,7 @@ module = {
>    name = tpm;
>    common = commands/tpm.c;
>    efi = commands/efi/tpm.c;
> +  efi = commands/efi/cc.c;
>    enable = efi;
>  };
>
> diff --git a/grub-core/commands/efi/cc.c b/grub-core/commands/efi/cc.c
> new file mode 100644
> index 000000000..0e6b417c4
> --- /dev/null
> +++ b/grub-core/commands/efi/cc.c
> @@ -0,0 +1,88 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2022  Free Software Foundation, Inc.
> + *
> + *  GRUB 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 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + *  EFI CC measurement support code.

You told me this feature is not Intel specific. In this case I would
move back this code to the grub-core/commands/efi/tpm.c. Sorry about
that...

> + */
> +
> +#include <grub/err.h>
> +#include <grub/i18n.h>
> +#include <grub/efi/api.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/cc.h>
> +#include <grub/tpm.h>
> +#include <grub/mm.h>
> +
> +static grub_efi_guid_t cc_measurement_guid = GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> +
> +static inline grub_err_t

Please drop inline from here. Compiler will know how to optimize this
function properly.

> +grub_efi_log_event_status (grub_efi_status_t status)
> +{
> +  switch (status)
> +    {
> +    case GRUB_EFI_SUCCESS:
> +      return 0;

return GRUB_ERR_NONE;

> +    case GRUB_EFI_DEVICE_ERROR:
> +      return grub_error (GRUB_ERR_IO, N_("Command failed"));

s/Command failed/command failed/

Most error messages should start with lowercase.

> +    case GRUB_EFI_INVALID_PARAMETER:
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Invalid parameter"));

s/Invalid parameter/invalid parameter/

> +    case GRUB_EFI_BUFFER_TOO_SMALL:
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too small"));

Ditto.

> +    case GRUB_EFI_NOT_FOUND:
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("TPM unavailable"));

However, this one is OK.

> +    default:
> +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("Unknown TPM error"));

s/Unknown TPM error/unknown TPM error/

> +    }
> +}
> +
> +grub_err_t
> +grub_cc_log_event (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
> +		   const char *description)
> +{
> +  grub_efi_cc_event_t *event;
> +  grub_efi_status_t status;
> +  grub_efi_cc_protocol_t *cc;
> +  grub_efi_cc_mr_index_t mr;
> +
> +  cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL);

I am not convinced calling into firmware on every event is good
idea. I think we should avoid that especially if at tpm module init
protocol was not available. On the other hand if protocol was available
during tpm module init is it safe to assume it is still available during
an tpm event? If not we can call grub_efi_locate_protocol() on every
event until return value is not NULL.

> +  if (cc == NULL)
> +    return 0;

return GRUB_ERR_NONE;

> +  status = efi_call_3 (cc->map_pcr_to_mr_index, cc, pcr, &mr);
> +  if (status != GRUB_EFI_SUCCESS)
> +    return grub_efi_log_event_status (status);

I do not fully understand that. You return an error and later the
return value from grub_cc_log_event() call is completely ignored.
I think you should decide how to cope with errors first and ignore
them in this function. e.g. return void, or process errors from
grub_cc_log_event() call in the caller accordingly.

> +  event = grub_zalloc (sizeof (grub_efi_cc_event_t)
> +		       + grub_strlen (description) + 1);
> +  if (event == NULL)
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> +		       N_ ("cannot allocate CC event buffer"));

You do not need space after N_. Yeah, I know it is confusing but it is
what it is. Sorry...

> +  event->Header.HeaderSize = sizeof (grub_efi_cc_event_header_t);
> +  event->Header.HeaderVersion = GRUB_EFI_CC_EVENT_HEADER_VERSION;
> +  event->Header.MrIndex = mr;
> +  event->Header.EventType = EV_IPL;
> +  event->Size = sizeof (*event) - sizeof (event->Event)

You will not need '- sizeof (event->Event)' if you define Event[0],
yes, 0 size, member in the grub_efi_cc_event_t.

> +		+ grub_strlen (description) + 1;
> +  grub_memcpy (event->Event, description, grub_strlen (description) + 1);

I think grub_strcpy() would be more natural here. And you do not need
copy '\0' because grub_zalloc() did work for you earlier.

> +  status = efi_call_5 (cc->hash_log_extend_event, cc, 0,
> +		       (grub_efi_physical_address_t) buf,
> +		       (grub_efi_uint64_t) size, event);
> +  grub_free (event);
> +
> +  return grub_efi_log_event_status (status);
> +}
> diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
> index a97d85368..c40093fb4 100644
> --- a/grub-core/commands/efi/tpm.c
> +++ b/grub-core/commands/efi/tpm.c
> @@ -22,6 +22,7 @@
>  #include <grub/i18n.h>
>  #include <grub/efi/api.h>
>  #include <grub/efi/efi.h>
> +#include <grub/efi/cc.h>
>  #include <grub/efi/tpm.h>
>  #include <grub/mm.h>
>  #include <grub/tpm.h>
> @@ -228,6 +229,8 @@ grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
>    grub_efi_handle_t tpm_handle;
>    grub_efi_uint8_t protocol_version;
>
> +  grub_cc_log_event(buf, size, pcr, description);

... and here you are ignoring return value...

Daniel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH V2] Enable TDX measurement to RTMR register
  2022-04-27 14:36 ` Daniel Kiper
@ 2022-04-29  4:25   ` Lu, Ken
  2022-04-29  6:15     ` Gerd Hoffmann
  0 siblings, 1 reply; 5+ messages in thread
From: Lu, Ken @ 2022-04-29  4:25 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Xu, Min M

Hi Kiper, 

Thanks for your detail review! Appreciate!!

I will continue fix all your suggestions except changing Event[1] => Event[0] for grub_efi_cc_event_t. Actually, I think your suggestion is very nice C programming tip, it will be more concise and readable. 
But I am thinking how about align with the definition in UEFI now https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c631235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100. I will talk with UEFI owner to make same changes for Event[0] both in EFI and grub/shim together in future. Do you agree?

Thanks
Ken

> -----Original Message-----
> From: Grub-devel <grub-devel-bounces+ken.lu=intel.com@gnu.org> On Behalf
> Of Daniel Kiper
> Sent: Wednesday, April 27, 2022 10:37 PM
> To: Lu, Ken <ken.lu@intel.com>
> Cc: grub-devel@gnu.org; Xu, Min M <min.m.xu@intel.com>
> Subject: Re: [PATCH V2] Enable TDX measurement to RTMR register
> 
> First of all, sorry for late reply but I am busy...
> 
> On Mon, Mar 14, 2022 at 02:52:22PM +0800, Lu Ken wrote:
> > Intel Trust Domain Extensions(Intel TDX) refers to an Intel technology
> > that extends Virtual Machine Extensions(VMX) and Multi-Key Total
> > Memory
> > Encryption(MK-TME) with a new kind of virtual machine guest called a
> > Trust Domain(TD)[1]. A TD runs in a CPU mode that protects the
> > confidentiality of its memory contents and its CPU state from any
> > other software, including the hosting Virtual Machine Monitor (VMM).
> >
> > Trust Domain Virtual Firmware (TDVF) is required to provide TD
> > services to the TD guest OS.[2] Its reference code is available at
> https://github.com/tianocore/edk2-staging/tree/TDVF.
> >
> > To support TD measurement/attestation, TDs provide 4 RTMR registers
> > like
> > TPM/TPM2 PCR as below:
> > - RTMR[0] is for TDVF configuration
> > - RTMR[1] is for the TD OS loader and kernel
> > - RTMR[2] is for the OS application
> > - RTMR[3] is reserved for special usage only
> >
> > This patch adds TD Measurement protocol support along with TPM/TPM2
> protocol.
> >
> > References:
> > [1]
> > https://software.intel.com/content/dam/develop/external/us/en/document
> > s/tdx-whitepaper-v4.pdf [2]
> > https://software.intel.com/content/dam/develop/external/us/en/document
> > s/tdx-virtual-firmware-design-guide-rev-1.pdf
> >
> > Signed-off-by: Lu Ken <ken.lu@intel.com>
> > ---
> >
> > Notes:
> >     v2 changes:
> >     - Separate the CC_MEASUREMENT code to standalone file grub-
> core/commands/efi/cc.c
> >     - Use NULL explicity for the judgement like "if (cc != NULL)"
> >     - Include all headers for all types, structure used in cc.h
> >     - Add the prefix "GRUB_"for all macro definition and the prefix "grub_" for
> type or structure's field
> >     - Align one argument in one line
> >
> >  grub-core/Makefile.core.def  |   1 +
> >  grub-core/commands/efi/cc.c  |  88 +++++++++++++++++++
> >  grub-core/commands/efi/tpm.c |   3 +
> >  include/grub/efi/cc.h        | 164
> +++++++++++++++++++++++++++++++++++
> >  4 files changed, 256 insertions(+)
> >  create mode 100644 grub-core/commands/efi/cc.c  create mode 100644
> > include/grub/efi/cc.h
> >
> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 6b00eb555..dbae796a7 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -2561,6 +2561,7 @@ module = {
> >    name = tpm;
> >    common = commands/tpm.c;
> >    efi = commands/efi/tpm.c;
> > +  efi = commands/efi/cc.c;
> >    enable = efi;
> >  };
> >
> > diff --git a/grub-core/commands/efi/cc.c b/grub-core/commands/efi/cc.c
> > new file mode 100644 index 000000000..0e6b417c4
> > --- /dev/null
> > +++ b/grub-core/commands/efi/cc.c
> > @@ -0,0 +1,88 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2022  Free Software Foundation, Inc.
> > + *
> > + *  GRUB 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 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + *  EFI CC measurement support code.
> 
> You told me this feature is not Intel specific. In this case I would move back this
> code to the grub-core/commands/efi/tpm.c. Sorry about that...
[Lu, Ken] Sure, I will move it.

> 
> > + */
> > +
> > +#include <grub/err.h>
> > +#include <grub/i18n.h>
> > +#include <grub/efi/api.h>
> > +#include <grub/efi/efi.h>
> > +#include <grub/efi/cc.h>
> > +#include <grub/tpm.h>
> > +#include <grub/mm.h>
> > +
> > +static grub_efi_guid_t cc_measurement_guid =
> > +GRUB_EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> > +
> > +static inline grub_err_t
> 
> Please drop inline from here. Compiler will know how to optimize this function
> properly.
[Lu, Ken] Thanks for reminder, I will fix it

> 
> > +grub_efi_log_event_status (grub_efi_status_t status) {
> > +  switch (status)
> > +    {
> > +    case GRUB_EFI_SUCCESS:
> > +      return 0;
> 
> return GRUB_ERR_NONE;
[Lu, Ken] Thanks for reminder, I will fix it

> 
> > +    case GRUB_EFI_DEVICE_ERROR:
> > +      return grub_error (GRUB_ERR_IO, N_("Command failed"));
> 
> s/Command failed/command failed/
[Lu, Ken] Thanks for this, I will fix it

> 
> Most error messages should start with lowercase.
> 
> > +    case GRUB_EFI_INVALID_PARAMETER:
> > +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Invalid
> > + parameter"));
> 
> s/Invalid parameter/invalid parameter/
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    case GRUB_EFI_BUFFER_TOO_SMALL:
> > +      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("Output buffer too
> > + small"));
> 
> Ditto.
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    case GRUB_EFI_NOT_FOUND:
> > +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("TPM
> > + unavailable"));
> 
> However, this one is OK.
> 
> > +    default:
> > +      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("Unknown TPM
> > + error"));
> 
> s/Unknown TPM error/unknown TPM error/
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +    }
> > +}
> > +
> > +grub_err_t
> > +grub_cc_log_event (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
> > +		   const char *description)
> > +{
> > +  grub_efi_cc_event_t *event;
> > +  grub_efi_status_t status;
> > +  grub_efi_cc_protocol_t *cc;
> > +  grub_efi_cc_mr_index_t mr;
> > +
> > +  cc = grub_efi_locate_protocol (&cc_measurement_guid, NULL);
> 
> I am not convinced calling into firmware on every event is good idea. I think we
> should avoid that especially if at tpm module init protocol was not available. On
> the other hand if protocol was available during tpm module init is it safe to
> assume it is still available during an tpm event? If not we can call
> grub_efi_locate_protocol() on every event until return value is not NULL.
[Lu, Ken] I think this is very nice suggestion. For TPM1/TPM2/CC, they all need call EFI to get handle, then call EFI to open protocol.
So we should only do once for both getting handle + open protocol. So I will optimize code for TPM1/TPM2 and CC according to your suggestion.

> 
> > +  if (cc == NULL)
> > +    return 0;
> 
> return GRUB_ERR_NONE;
[Lu, Ken] Thanks for this comment, I will fix it

> 
> > +  status = efi_call_3 (cc->map_pcr_to_mr_index, cc, pcr, &mr);  if
> > + (status != GRUB_EFI_SUCCESS)
> > +    return grub_efi_log_event_status (status);
> 
> I do not fully understand that. You return an error and later the return value
> from grub_cc_log_event() call is completely ignored.
> I think you should decide how to cope with errors first and ignore them in this
> function. e.g. return void, or process errors from
> grub_cc_log_event() call in the caller accordingly.
[Lu, Ken] Thanks for this reminder! I will return void, since the error of grub_cc_log_event() should not block other TPM log event, because they can coexist.

> 
> > +  event = grub_zalloc (sizeof (grub_efi_cc_event_t)
> > +		       + grub_strlen (description) + 1);
> > +  if (event == NULL)
> > +    return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> > +		       N_ ("cannot allocate CC event buffer"));
> 
> You do not need space after N_. Yeah, I know it is confusing but it is what it is.
> Sorry...
[Lu, Ken] Thanks for this, I will fix it.

> 
> > +  event->Header.HeaderSize = sizeof (grub_efi_cc_event_header_t);
> > + event->Header.HeaderVersion = GRUB_EFI_CC_EVENT_HEADER_VERSION;
> > + event->Header.MrIndex = mr;  event->Header.EventType = EV_IPL;
> > + event->Size = sizeof (*event) - sizeof (event->Event)
> 
> You will not need '- sizeof (event->Event)' if you define Event[0], yes, 0 size,
> member in the grub_efi_cc_event_t.
[Lu, Ken] Very nice C programming tip, it will be more concise and readable. Could we keep this Event[1] now firstly aligning with UEFI's definition at https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c631235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100, I will talk with UEFI owner to make same changes in future.

> 
> > +		+ grub_strlen (description) + 1;
> > +  grub_memcpy (event->Event, description, grub_strlen (description) +
> > +1);
> 
> I think grub_strcpy() would be more natural here. And you do not need copy '\0'
> because grub_zalloc() did work for you earlier.
[Lu, Ken] Thanks for this comment, will fix it.

> 
> > +  status = efi_call_5 (cc->hash_log_extend_event, cc, 0,
> > +		       (grub_efi_physical_address_t) buf,
> > +		       (grub_efi_uint64_t) size, event);
> > +  grub_free (event);
> > +
> > +  return grub_efi_log_event_status (status); }
> > diff --git a/grub-core/commands/efi/tpm.c
> > b/grub-core/commands/efi/tpm.c index a97d85368..c40093fb4 100644
> > --- a/grub-core/commands/efi/tpm.c
> > +++ b/grub-core/commands/efi/tpm.c
> > @@ -22,6 +22,7 @@
> >  #include <grub/i18n.h>
> >  #include <grub/efi/api.h>
> >  #include <grub/efi/efi.h>
> > +#include <grub/efi/cc.h>
> >  #include <grub/efi/tpm.h>
> >  #include <grub/mm.h>
> >  #include <grub/tpm.h>
> > @@ -228,6 +229,8 @@ grub_tpm_measure (unsigned char *buf, grub_size_t
> size, grub_uint8_t pcr,
> >    grub_efi_handle_t tpm_handle;
> >    grub_efi_uint8_t protocol_version;
> >
> > +  grub_cc_log_event(buf, size, pcr, description);
> 
> ... and here you are ignoring return value...
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] Enable TDX measurement to RTMR register
  2022-04-29  4:25   ` Lu, Ken
@ 2022-04-29  6:15     ` Gerd Hoffmann
  2022-04-30  0:41       ` Lu, Ken
  0 siblings, 1 reply; 5+ messages in thread
From: Gerd Hoffmann @ 2022-04-29  6:15 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Xu, Min M

  Hi,

> But I am thinking how about align with the definition in UEFI now
> https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c631235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100.

It surely makes sense to update the uefi specs too.

> I will talk with UEFI owner to make same changes for Event[0] both in
> EFI and grub/shim together in future. Do you agree?

Given that grub manages it's own copy of the structs anyway (instead of
copying them over from edk2) I don't see a need to synchronize the
changes.  The struct layout doesn't change, so this doesn't break
compatibility.

take care,
  Gerd



^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH V2] Enable TDX measurement to RTMR register
  2022-04-29  6:15     ` Gerd Hoffmann
@ 2022-04-30  0:41       ` Lu, Ken
  0 siblings, 0 replies; 5+ messages in thread
From: Lu, Ken @ 2022-04-30  0:41 UTC (permalink / raw)
  To: The development of GNU GRUB; +Cc: Xu, Min M

Hi Gerd,
	Thanks for your comment, make sense! So I will update grub's structure according to Kiper's suggestion!

Thanks	
Ken

> -----Original Message-----
> From: Grub-devel <grub-devel-bounces+ken.lu=intel.com@gnu.org> On Behalf
> Of Gerd Hoffmann
> Sent: Friday, April 29, 2022 2:16 PM
> To: The development of GNU GRUB <grub-devel@gnu.org>
> Cc: Xu, Min M <min.m.xu@intel.com>
> Subject: Re: [PATCH V2] Enable TDX measurement to RTMR register
> 
>   Hi,
> 
> > But I am thinking how about align with the definition in UEFI now
> >
> https://github.com/tianocore/edk2/blob/d372ab585a2cdc5348af5f701c56c63
> 1235fe698/MdePkg/Include/Protocol/CcMeasurement.h#L100.
> 
> It surely makes sense to update the uefi specs too.
> 
> > I will talk with UEFI owner to make same changes for Event[0] both in
> > EFI and grub/shim together in future. Do you agree?
> 
> Given that grub manages it's own copy of the structs anyway (instead of copying
> them over from edk2) I don't see a need to synchronize the changes.  The
> struct layout doesn't change, so this doesn't break compatibility.
> 
> take care,
>   Gerd
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-04-30  0:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14  6:52 [PATCH V2] Enable TDX measurement to RTMR register Lu Ken
2022-04-27 14:36 ` Daniel Kiper
2022-04-29  4:25   ` Lu, Ken
2022-04-29  6:15     ` Gerd Hoffmann
2022-04-30  0:41       ` Lu, Ken

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.