[PATCH 0/6] Measuring TPM update counter in IMA

[PATCH 0/6] Measuring TPM update counter in IMA

[Tushar Sugandhi]

Entries in IMA log may be lost due to code bugs, certain error conditions
being met etc.  This can result in TPM PCRs getting out of sync with the
IMA log.  One such example is events between kexec 'load' and kexec
'execute' getting lost from the IMA log when the system soft-boots into
the new Kernel using kexec[1].  The remote attestation service does not
have any information if the PCR mismatch with IMA log is because of loss
of entries in the IMA log or something else.  TPM 2.0 provides an update
counter which is incremented each time a PCR is updated [2]. Measuring the
TPM PCR update counter in IMA subsystem will help the remote attestation
service to validate if there are any missing entries in the IMA log, when
the system goes through certain important state changes (e.g. kexec soft
boot, IMA log snapshotting etc.)

This patch series provides the required functionality to measure the
update counter through IMA subsystem by -
 - introducing a function to retrieve PCR update counter in the TPM
   subsystem.
 - IMA functionality to acquire the update counter from the TPM subsystem.
 - Measuring the update counter at system boot and at kexec Kernel
   load.


This patch series would be a prerequisite for the next version of kexec
load/execute series[1] and the future IMA log snapshotting patch series.

[1] https://lore.kernel.org/all/20230703215709.1195644-1-tusharsu@linux.microsoft.com/
    ima: measure events between kexec load and execute

[2] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
    Section 22.4.2, Page 206.

Tushar Sugandhi (6):
  tpm: implement TPM2 function to get update counter
  tpm: provide functionality to get update counter
  ima: get TPM update counter
  ima: implement functionality to measure TPM update counter
  ima: measure TPM update counter at ima_init
  kexec: measure TPM update counter in ima log at kexec load

 drivers/char/tpm/tpm-interface.c   | 28 +++++++++++++++++
 drivers/char/tpm/tpm.h             |  3 ++
 drivers/char/tpm/tpm2-cmd.c        | 48 ++++++++++++++++++++++++++++++
 include/linux/ima.h                |  1 +
 include/linux/tpm.h                |  8 +++++
 kernel/kexec_file.c                |  3 ++
 security/integrity/ima/ima.h       |  2 ++
 security/integrity/ima/ima_init.c  |  3 ++
 security/integrity/ima/ima_main.c  | 29 ++++++++++++++++++
 security/integrity/ima/ima_queue.c | 16 ++++++++++
 10 files changed, 141 insertions(+)

-- 
2.25.1

[PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

The TPM2_PCR_Read command returns TPM2_PCR_Read Response struct[1].  It
contains pcrUpdateCounter member which contains the current value of TPM
PCR update counter.  The update counter provides the number of times the
PCRs are updated, which is essential for tracking changes and verifying
system integrity.  Thus, subsystems (like IMA) should measure
pcrUpdateCounter value.  Although tpm2_pcr_read_out struct is returned
by tpm2_pcr_read(), it is not used by it's caller function tpm_pcr_read().
Further, TPM2_PCR_Read Response struct and pcrUpdateCounter is not
available in tpm1_pcr_read().

PcrUpdateCounter is only needed in a specific case (IMA for measurements).
Changing tpm_pcr_read() and tpm2_pcr_read() function signature to return
tpm2_pcr_read_out struct would be a more disruptive change, since these
functions are used elsewhere too.  Creating separate functions to get
pcrUpdateCounter when needed would be a cleaner approach.

Add a function, 'tpm2_pcr_get_update_counter()' to retrieve
the update counter for a given PCR index and algorithm ID on a TPM2 chip.

This function complements existing TPM functionalities such as reading
and extending PCRs, and enhances the ability to monitor PCR status
in the Linux Kernel. 

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
Section 22.4.2, Page 206.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 drivers/char/tpm/tpm.h      |  3 +++
 drivers/char/tpm/tpm2-cmd.c | 48 +++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 830014a26609..60489f21d3bd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -288,6 +288,9 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
 int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		  struct tpm_digest *digest, u16 *digest_size_ptr);
+int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
+				u32 pcr_idx, u16 alg_id,
+				u32 *update_counter);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 93545be190a5..55f4e102289a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -216,6 +216,54 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 	return rc;
 }
 
+/**
+ * tpm2_pcr_get_update_counter() - gets an update counter value for a PCR bank
+ * @chip: TPM chip to use
+ * @pcr_idx: PCR index used to retrieve the update counter
+ * @alg_id:	alg id used to retrieve the update counter
+ * @update_counter: output update counter value
+ *
+ * Return: Same as with tpm_transmit_cmd.
+ */
+int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
+				u32 pcr_idx, u16 alg_id, u32 *update_counter)
+{
+	int rc;
+	struct tpm_buf buf;
+	struct tpm2_pcr_read_out *read_out;
+	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
+
+	if (pcr_idx >= TPM2_PLATFORM_PCR)
+		return -EINVAL;
+
+	if (!update_counter)
+		return -EINVAL;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
+	if (rc)
+		return rc;
+
+	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
+
+	tpm_buf_append_u32(&buf, 1);
+	tpm_buf_append_u16(&buf, alg_id);
+	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
+	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
+		       sizeof(pcr_select));
+
+	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
+	if (rc)
+		goto out;
+
+	read_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
+
+	*update_counter = be32_to_cpu(read_out->update_cnt);
+
+out:
+	tpm_buf_destroy(&buf);
+	return rc;
+}
+
 struct tpm2_null_auth_area {
 	__be32  handle;
 	__be16  nonce_size;
-- 
2.25.1

[PATCH 2/6] tpm: provide functionality to get update counter

[Tushar Sugandhi]

The IMA subsystem needs to measure pcrUpdateCounter value present in 
TPM2_PCR_Read Response struct [1].  However,the  pcrUpdateCounter value
is not exposed outside of the TPM subsystem by any of the existing
functions. 

Implement a new function 'tpm_pcr_get_update_counter()',  which provides
a way to retrieve the PCR update counter values from subsystems outside 
of TPM.  If the input tpm_chip is not a TPM2 chip, return an error as
the functionality is currently only implemented for TPM2 chips.

This function improves TPM capabilities in the Linux kernel by
facilitating access to the PCR update counter.

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
Section 22.4.2, Page 206.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 drivers/char/tpm/tpm-interface.c | 28 ++++++++++++++++++++++++++++
 include/linux/tpm.h              |  8 ++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7e513b771832..9a1088914487 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -300,6 +300,34 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 }
 EXPORT_SYMBOL_GPL(tpm_pcr_read);
 
+/**
+ * tpm_pcr_get_update_counter - gets an update counter value for a PCR bank
+ * @chip: a &struct tpm_chip instance, %NULL for the default chip
+ * @pcr_idx: PCR index used to retrieve the update counter
+ * @alg_id:	alg id used to retrieve the update counter
+ * @update_counter: output update counter value
+ *
+ * Return: same as with tpm_transmit_cmd()
+ */
+int tpm_pcr_get_update_counter(struct tpm_chip *chip, u32 pcr_idx,
+		 u16 alg_id, u32 *update_counter)
+{
+	int rc;
+
+	chip = tpm_find_get_ops(chip);
+	if (!chip)
+		return -ENODEV;
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		rc = tpm2_pcr_get_update_counter(chip, pcr_idx, alg_id,
+						 update_counter);
+	else
+		rc = -ENODEV;
+
+	tpm_put_ops(chip);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_get_update_counter);
 /**
  * tpm_pcr_extend - extend a PCR value in SHA1 bank.
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4dc97b9f65fb..3b55218b70fa 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -424,6 +424,8 @@ extern ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
 				size_t min_rsp_body_length, const char *desc);
 extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 			struct tpm_digest *digest);
+extern int tpm_pcr_get_update_counter(struct tpm_chip *chip, u32 pcr_idx,
+		 u16 alg_id, u32 *update_counter);
 extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 			  struct tpm_digest *digests);
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
@@ -440,6 +442,12 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
 {
 	return -ENODEV;
 }
+static inline int tpm_pcr_get_update_counter(struct tpm_chip *chip,
+					     u32 pcr_idx, u16 alg_id,
+					     u32 *update_counter)
+{
+	return -ENODEV;
+}
 
 static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 				 struct tpm_digest *digests)
-- 
2.25.1

[PATCH 3/6] ima: get TPM update counter

[Tushar Sugandhi]

Measuring the TPM PCR update counter will help the remote attestation
service to validate if there are any missing entries in the IMA log, when
the system goes through certain important state changes (e.g. kexec soft
boot, IMA log snapshotting etc.).  Detecting such missing entries would
help the remote attestation service functionality to be more robust.
It should also help the system administrators with manual investigations
when TPM PCR quotes go out of sync with IMA measurements.

Implement a new function, 'ima_tpm_get_update_counter()', which uses
the 'tpm_pcr_get_update_counter()' function from the TPM driver interface
to retrieve the PCR update counter of the TPM chip in use.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h       |  1 +
 security/integrity/ima/ima_queue.c | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c29db699c996..4acd0e5a830f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -167,6 +167,7 @@ void ima_init_template_list(void);
 int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
+int ima_tpm_get_update_counter(u32 *cpu_update_counter);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 532da87ce519..38f5c35b23b2 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -135,6 +135,22 @@ unsigned long ima_get_binary_runtime_size(void)
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);
 }
 
+int ima_tpm_get_update_counter(u32 *update_counter)
+{
+	int result;
+
+	if (!update_counter)
+		return -EINVAL;
+
+	result = tpm_pcr_get_update_counter(ima_tpm_chip,
+				CONFIG_IMA_MEASURE_PCR_IDX, TPM_ALG_SHA1, update_counter);
+
+	if (result != 0)
+		pr_err("Failed to get TPM PCR update counter, result: %d\n", result);
+
+	return result;
+}
+
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
 	int result = 0;
-- 
2.25.1

[PATCH 4/6] ima: implement functionality to measure TPM update counter

[Tushar Sugandhi]

Currently TPM update counter is not available external to the system,
for instance, a remote attestation service.  It is a problem because
the service cannot easily determine if the IMA log entries are missing.
The IMA functionality needs to be extended to measure the TPM update
counter from various subsystems in Linux kernel to help detect if
the IMA log entries are missing.

Implement a function, 'ima_measure_update_counter()' which would retrieve
the TPM update counter using the previously defined function
'ima_tpm_get_update_counter()'.  Format it as a string with the value 
"update_counter=<N>;", and measure it using the function
'ima_measure_critical_data()'.

The function takes an event name as input, and the update counter value
is measured as part of this event.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 include/linux/ima.h               |  1 +
 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 86b57757c7b1..f15f3a6a4c72 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label,
 				     const char *event_name,
 				     const void *buf, size_t buf_len,
 				     bool hash, u8 *digest, size_t digest_len);
+int ima_measure_update_counter(const char *event_name);
 
 #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 extern void ima_appraise_parse_cmdline(void);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4acd0e5a830f..5484bd362237 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -168,6 +168,7 @@ int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
 int ima_tpm_get_update_counter(u32 *cpu_update_counter);
+int ima_measure_update_counter(const char *event_name);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d66a0a36415e..1bcd45cc5a6a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
 }
 EXPORT_SYMBOL_GPL(ima_measure_critical_data);
 
+#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
+int ima_measure_update_counter(const char *event_name)
+{
+	int result;
+	u32 update_counter = 0;
+	char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
+	int buf_len;
+
+	if (!event_name)
+		return -ENOPARAM;
+
+	result = ima_tpm_get_update_counter(&update_counter);
+
+	if (result != 0)
+		return result;
+
+	scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
+			  update_counter);
+
+	buf_len = strlen(buf);
+
+	result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
+				  buf, buf_len, false, NULL, 0);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(ima_measure_update_counter);
+
 static int __init init_ima(void)
 {
 	int error;
-- 
2.25.1

[PATCH 5/6] ima: measure TPM update counter at ima_init

[Tushar Sugandhi]

IMA log entries can be lost due to a variety of causes, such as code bugs
or error conditions, leading to a mismatch between TPM PCRs and
the IMA log.  Measuring TPM PCR update counter during ima_init would
provide a baseline counter for the number of times the TPM PCRs are
updated.  The remote attestation service can compare this baseline
counter with a subsequent measured one (e.g., post-kexec soft-boot) to
identify if there are any lost IMA log events.

Measure the TPM update counter at ima init.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_init.c | 3 +++
 security/integrity/ima/ima_main.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 63979aefc95f..9bb18d6c2fd6 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -154,5 +154,8 @@ int __init ima_init(void)
 				  UTS_RELEASE, strlen(UTS_RELEASE), false,
 				  NULL, 0);
 
+	/* Measures TPM update counter at ima_init */
+	ima_measure_update_counter("ima_init_tpm_update_counter");
+
 	return rc;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1bcd45cc5a6a..93357c245e82 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 				   buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
 				   NULL, false, NULL, 0);
 	fdput(f);
+
 }
 
 /**
-- 
2.25.1

[PATCH 6/6] kexec: measure TPM update counter in ima log at kexec load

[Tushar Sugandhi]

IMA measurements snapshot occurs at kexec 'load', but any additional
measurements between 'load' and kexec 'execute' aren't carried over
post kexec soft-reboot.[1] This may lead to TPM PCRs extending with
events that are not reflected in the new Kernel's IMA log.  By measuring
the TPM update counter at kexec 'load' and at ima_init after the kexec
soft-reboot, the remote attestation service can identify potentially
lost events by comparing the log event count with the counter difference.

Measure the TPM update counter at kexec image load.

[1] https://lore.kernel.org/all/20230703215709.1195644-1-tusharsu@linux.microsoft.com/
    ima: measure events between kexec load and execute

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 kernel/kexec_file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index f1a0e4e3fb5c..4b6391b02c5a 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -246,6 +246,9 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 				  image->cmdline_buf_len - 1);
 	}
 
+	/* Measures TPM update counter at kexec load. */
+	ima_measure_update_counter("kexec_load_tpm_update_counter");
+
 	/* IMA needs to pass the measurement list to the next kernel. */
 	ima_add_kexec_buffer(image);
 
-- 
2.25.1

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Jarkko Sakkinen]

The short summary is cryptic to say the least.

"update counter" does not map it to have anything to do with PCRs.

Why not "tpm: Read pcrUpdateCounter field from TPM2_PCR_Read"?

On Tue Aug 1, 2023 at 9:19 PM EEST, Tushar Sugandhi wrote:
> The TPM2_PCR_Read command returns TPM2_PCR_Read Response struct[1].  It
> contains pcrUpdateCounter member which contains the current value of TPM
> PCR update counter.  The update counter provides the number of times the
> PCRs are updated, which is essential for tracking changes and verifying
> system integrity.  Thus, subsystems (like IMA) should measure
> pcrUpdateCounter value.  Although tpm2_pcr_read_out struct is returned
> by tpm2_pcr_read(), it is not used by it's caller function tpm_pcr_read().
> Further, TPM2_PCR_Read Response struct and pcrUpdateCounter is not
> available in tpm1_pcr_read().
>
> PcrUpdateCounter is only needed in a specific case (IMA for measurements).
> Changing tpm_pcr_read() and tpm2_pcr_read() function signature to return
> tpm2_pcr_read_out struct would be a more disruptive change, since these
> functions are used elsewhere too.  Creating separate functions to get
> pcrUpdateCounter when needed would be a cleaner approach.
>
> Add a function, 'tpm2_pcr_get_update_counter()' to retrieve
> the update counter for a given PCR index and algorithm ID on a TPM2 chip.
>
> This function complements existing TPM functionalities such as reading
> and extending PCRs, and enhances the ability to monitor PCR status
> in the Linux Kernel. 
>
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
> Section 22.4.2, Page 206.
>
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  drivers/char/tpm/tpm.h      |  3 +++
>  drivers/char/tpm/tpm2-cmd.c | 48 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 830014a26609..60489f21d3bd 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -288,6 +288,9 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
>  int tpm2_get_timeouts(struct tpm_chip *chip);
>  int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  		  struct tpm_digest *digest, u16 *digest_size_ptr);
> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
> +				u32 pcr_idx, u16 alg_id,
> +				u32 *update_counter);

tpm_pcr_read_update_cnt()

>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  		    struct tpm_digest *digests);
>  int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 93545be190a5..55f4e102289a 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -216,6 +216,54 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  	return rc;
>  }
>  
> +/**
> + * tpm2_pcr_get_update_counter() - gets an update counter value for a PCR bank
> + * @chip: TPM chip to use
> + * @pcr_idx: PCR index used to retrieve the update counter
> + * @alg_id:	alg id used to retrieve the update counter
> + * @update_counter: output update counter value
> + *
> + * Return: Same as with tpm_transmit_cmd.
> + */
> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
> +				u32 pcr_idx, u16 alg_id, u32 *update_counter)
> +{
> +	int rc;
> +	struct tpm_buf buf;
> +	struct tpm2_pcr_read_out *read_out;
> +	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
> +
> +	if (pcr_idx >= TPM2_PLATFORM_PCR)
> +		return -EINVAL;
> +
> +	if (!update_counter)
> +		return -EINVAL;
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
> +	if (rc)
> +		return rc;
> +
> +	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
> +
> +	tpm_buf_append_u32(&buf, 1);
> +	tpm_buf_append_u16(&buf, alg_id);
> +	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
> +	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
> +		       sizeof(pcr_select));
> +
> +	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
> +	if (rc)
> +		goto out;
> +
> +	read_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
> +
> +	*update_counter = be32_to_cpu(read_out->update_cnt);
> +
> +out:
> +	tpm_buf_destroy(&buf);
> +	return rc;
> +}
> +
>  struct tpm2_null_auth_area {
>  	__be32  handle;
>  	__be16  nonce_size;
> -- 
> 2.25.1

BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

Thanks for the response Jarkko.

On 8/1/23 12:02, Jarkko Sakkinen wrote:
> The short summary is cryptic to say the least.
Do you mean the patch subject line, or the description below?
> "update counter" does not map it to have anything to do with PCRs.
Agreed.  I noticed that when I was testing the patches.
The update counter is same for all PCRs.  It was also the same for
the two hash algo's I tested it for (SHA1 and SHA256). But the spec
description and Kernel implementation requires to pass the
pcr_idx and hash algo to PCR_Read command to get the update counter.
> Why not "tpm: Read pcrUpdateCounter field from TPM2_PCR_Read"?
As I said in the patch description below, update counter is only
needed for IMA measurements.  None of the other code that calls
tpm2_pcr_read() use the update counter.

I was not sure if you were okay changing the function signature and
implementation of tpm2_pcr_read(). It felt disruptive.

But I can update tpm2_pcr_read() if you are ok with it.
Please let me know.

I also have a few more thoughts on this in the comment below.
> On Tue Aug 1, 2023 at 9:19 PM EEST, Tushar Sugandhi wrote:
>> The TPM2_PCR_Read command returns TPM2_PCR_Read Response struct[1].  It
>> contains pcrUpdateCounter member which contains the current value of TPM
>> PCR update counter.  The update counter provides the number of times the
>> PCRs are updated, which is essential for tracking changes and verifying
>> system integrity.  Thus, subsystems (like IMA) should measure
>> pcrUpdateCounter value.  Although tpm2_pcr_read_out struct is returned
>> by tpm2_pcr_read(), it is not used by it's caller function tpm_pcr_read().
>> Further, TPM2_PCR_Read Response struct and pcrUpdateCounter is not
>> available in tpm1_pcr_read().
>>
>> PcrUpdateCounter is only needed in a specific case (IMA for measurements).
>> Changing tpm_pcr_read() and tpm2_pcr_read() function signature to return
>> tpm2_pcr_read_out struct would be a more disruptive change, since these
>> functions are used elsewhere too.  Creating separate functions to get
>> pcrUpdateCounter when needed would be a cleaner approach.
>>
>> Add a function, 'tpm2_pcr_get_update_counter()' to retrieve
>> the update counter for a given PCR index and algorithm ID on a TPM2 chip.
>>
>> This function complements existing TPM functionalities such as reading
>> and extending PCRs, and enhances the ability to monitor PCR status
>> in the Linux Kernel.
>>
>> [1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
>> Section 22.4.2, Page 206.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   drivers/char/tpm/tpm.h      |  3 +++
>>   drivers/char/tpm/tpm2-cmd.c | 48 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 830014a26609..60489f21d3bd 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -288,6 +288,9 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
>>   int tpm2_get_timeouts(struct tpm_chip *chip);
>>   int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   		  struct tpm_digest *digest, u16 *digest_size_ptr);
>> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
>> +				u32 pcr_idx, u16 alg_id,
>> +				u32 *update_counter);
> tpm_pcr_read_update_cnt()
I can rename 'get' -> 'read'

About 'tpm2' -> 'tpm':
I already have tpm_pcr_get_update_counter() defined in patch 2.

I was following the existing pattern here in patch 1 and 2 i. e.
  - Implementing the tpm1/tpm2 specific functionality in
    drivers/char/tpm/tpm.h, drivers/char/tpm/tpm2-cmd.c,
    drivers/char/tpm/tpm1-cmd.c.

  - And combining that functionality in drivers/char/tpm/tpm-interface.c
    and exposing it to other subsystems (like IMA) through 
include/linux/tpm.h
    (patch 2 of this series)

BTW, if I understand correctly, the update counter is not available in 
TPM 1.2.

Please let me know if you want me to expose the functionality directly
from drivers/char/tpm/tpm2-cmd.c and getting rid of patch #2 of this series.

~Tushar

>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>>   		    struct tpm_digest *digests);
>>   int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 93545be190a5..55f4e102289a 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -216,6 +216,54 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   	return rc;
>>   }
>>   
>> +/**
>> + * tpm2_pcr_get_update_counter() - gets an update counter value for a PCR bank
>> + * @chip: TPM chip to use
>> + * @pcr_idx: PCR index used to retrieve the update counter
>> + * @alg_id:	alg id used to retrieve the update counter
>> + * @update_counter: output update counter value
>> + *
>> + * Return: Same as with tpm_transmit_cmd.
>> + */
>> +int tpm2_pcr_get_update_counter(struct tpm_chip *chip,
>> +				u32 pcr_idx, u16 alg_id, u32 *update_counter)
>> +{
>> +	int rc;
>> +	struct tpm_buf buf;
>> +	struct tpm2_pcr_read_out *read_out;
>> +	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>> +
>> +	if (pcr_idx >= TPM2_PLATFORM_PCR)
>> +		return -EINVAL;
>> +
>> +	if (!update_counter)
>> +		return -EINVAL;
>> +
>> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>> +	if (rc)
>> +		return rc;
>> +
>> +	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
>> +
>> +	tpm_buf_append_u32(&buf, 1);
>> +	tpm_buf_append_u16(&buf, alg_id);
>> +	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
>> +	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
>> +		       sizeof(pcr_select));
>> +
>> +	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
>> +	if (rc)
>> +		goto out;
>> +
>> +	read_out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>> +
>> +	*update_counter = be32_to_cpu(read_out->update_cnt);
>> +
>> +out:
>> +	tpm_buf_destroy(&buf);
>> +	return rc;
>> +}
>> +
>>   struct tpm2_null_auth_area {
>>   	__be32  handle;
>>   	__be16  nonce_size;
>> -- 
>> 2.25.1
> BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Jarkko Sakkinen]

On Wed Aug 2, 2023 at 12:01 AM EEST, Tushar Sugandhi wrote:
> Thanks for the response Jarkko.
>
> On 8/1/23 12:02, Jarkko Sakkinen wrote:
> > The short summary is cryptic to say the least.
> Do you mean the patch subject line, or the description below?

It is in the process documentation:

https://www.kernel.org/doc/html/v6.3/process/submitting-patches.html#the-canonical-patch-format

> > "update counter" does not map it to have anything to do with PCRs.
> Agreed.  I noticed that when I was testing the patches.
> The update counter is same for all PCRs.  It was also the same for
> the two hash algo's I tested it for (SHA1 and SHA256). But the spec
> description and Kernel implementation requires to pass the
> pcr_idx and hash algo to PCR_Read command to get the update counter.

I was referring to the fact that TPM2_PCR_Read does not have a field
called "update counter" in its response but it has a field called
"pcrUpdateCounter". Please refer to thigs that actually exist.

In the long description you are in some occasions referring to the same
object as:

1. "update counter"
2. "pcrUpdateCounter"
3. "PcrUpdateCounter"

This is ambiguous and wrong.

From long description I see zero motivation to ack this change, except
some heresay about IMA requiring it. Why does IMA need update_cnt and
why this is not documented to the long description?

> But I can update tpm2_pcr_read() if you are ok with it.
> Please let me know.

You can add "u32 *update_cnt".

BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

On 8/1/23 20:58, Jarkko Sakkinen wrote:
> On Wed Aug 2, 2023 at 12:01 AM EEST, Tushar Sugandhi wrote:
>> Thanks for the response Jarkko.
>>
>> On 8/1/23 12:02, Jarkko Sakkinen wrote:
>>> The short summary is cryptic to say the least.
>> Do you mean the patch subject line, or the description below?
> It is in the process documentation:
>
> https://www.kernel.org/doc/html/v6.3/process/submitting-patches.html#the-canonical-patch-format
Sounds good.  I will cleanup both the summary phrase and the patch 
description.
>>> "update counter" does not map it to have anything to do with PCRs.
>> Agreed.  I noticed that when I was testing the patches.
>> The update counter is same for all PCRs.  It was also the same for
>> the two hash algo's I tested it for (SHA1 and SHA256). But the spec
>> description and Kernel implementation requires to pass the
>> pcr_idx and hash algo to PCR_Read command to get the update counter.
> I was referring to the fact that TPM2_PCR_Read does not have a field
> called "update counter" in its response but it has a field called
> "pcrUpdateCounter". Please refer to thigs that actually exist.
>
> In the long description you are in some occasions referring to the same
> object as:
>
> 1. "update counter"
> 2. "pcrUpdateCounter"
> 3. "PcrUpdateCounter"
>
> This is ambiguous and wrong.
Thanks. I will consistently use pcrUpdateCounter going forward.
> >From long description I see zero motivation to ack this change, except
> some heresay about IMA requiring it. Why does IMA need update_cnt and
> why this is not documented to the long description?
Since patch 2 of this series exposes the functionality to IMA,
it is described in the long description of patch 2.

But I can add the description here as well for completeness.
>> But I can update tpm2_pcr_read() if you are ok with it.
>> Please let me know.
> You can add "u32 *update_cnt".
Sounds good.  Will do.

Btw, the function tpm2_pcr_read is not exposed directly to the other
subsystems (like IMA).  It is exposed via tpm_pcr_read.

Do you want to expose tpm2_pcr_read directly,
or do you want me to update the function signature of tpm_pcr_read as well?

Updating the function signature of tpm_pcr_read as well -
to return "u32 *update_cnt" seems like the right approach.
In that case, I can set *update_cnt to say 0 or -1 for TPM1
(because pcrUpdateCounter is not available for TPM1).

Please let me know what do you think.

I will make the changes accordingly.

I will also wait for IMA/Kexec maintainers to take a look at the 
remaining patches
in this series, incorporate their feedback, and send the V2 of this series.

Thanks again for your feedback. Really appreciate it.

~Tushar
>
> BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Mimi Zohar]

On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote:
> 
> From long description I see zero motivation to ack this change, except
> some heresay about IMA requiring it. Why does IMA need update_cnt and
> why this is not documented to the long description?

The motivation is to detect whether the IMA measurement list has been
truncated, for whatever reason.  A new IMA record should be defined
containing the "pcrCounter" value.  (I have not had a chance to review
this patch set.)

This new record would be a pre-req for both Tushar's "ima: measure
events between kexec load and execute" patch set and Sush's proposal to
trim the measurement list.  (I haven't looked at it yet either.)

-- 
thanks,

Mimi

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Jarkko Sakkinen]

On Thu Aug 3, 2023 at 12:04 AM EEST, Tushar Sugandhi wrote:
> Btw, the function tpm2_pcr_read is not exposed directly to the other
> subsystems (like IMA).  It is exposed via tpm_pcr_read.
>
> Do you want to expose tpm2_pcr_read directly,
> or do you want me to update the function signature of tpm_pcr_read as well?

As long as you mention that 'update_cnt' causes divegence in the
in-kernel API, and therefore tpm[12]_pcr_read() cannnot be under the
same orchestrator.

If you take this path, please implement a precursory patch, which
replace the existing call sites with some sequence of tpm[12]_pcr_read()
and tpm_is_tpm2() calls.

BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Jarkko Sakkinen]

On Thu Aug 3, 2023 at 4:22 AM EEST, Mimi Zohar wrote:
> On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote:
> > 
> > From long description I see zero motivation to ack this change, except
> > some heresay about IMA requiring it. Why does IMA need update_cnt and
> > why this is not documented to the long description?
>
> The motivation is to detect whether the IMA measurement list has been
> truncated, for whatever reason.  A new IMA record should be defined
> containing the "pcrCounter" value.  (I have not had a chance to review
> this patch set.)
>
> This new record would be a pre-req for both Tushar's "ima: measure
> events between kexec load and execute" patch set and Sush's proposal to
> trim the measurement list.  (I haven't looked at it yet either.)

Please describe the story in a bit more understandable form. In the
commit messages it is not good to have some redundancy in patch sets.

BR, Jarkko

Re: [PATCH 0/6] Measuring TPM update counter in IMA

[Stefan Berger]

On 8/1/23 14:19, Tushar Sugandhi wrote:
> Entries in IMA log may be lost due to code bugs, certain error conditions

I hope we don't have such bugs. And I guess the most critical ones would be
between logging and PCR extensions

> being met etc.  This can result in TPM PCRs getting out of sync with the
> IMA log.  One such example is events between kexec 'load' and kexec
> 'execute' getting lost from the IMA log when the system soft-boots into
> the new Kernel using kexec[1].  The remote attestation service does not

Though this particular condition I thought would go away with your kexec series.

The other conditions would be an out-of-memory or TPM failure. The OOM would
probably be more critical since something that was supposed to be logged
couldn't be logged and now you cannot show this anymore and presumably not even
an error condition could be logged.

https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179

> have any information if the PCR mismatch with IMA log is because of loss
> of entries in the IMA log or something else.  TPM 2.0 provides an update
> counter which is incremented each time a PCR is updated [2]. Measuring the
> TPM PCR update counter in IMA subsystem will help the remote attestation
> service to validate if there are any missing entries in the IMA log, when



> the system goes through certain important state changes (e.g. kexec soft
> boot, IMA log snapshotting etc.)
> 
> This patch series provides the required functionality to measure the
> update counter through IMA subsystem by -
>   - introducing a function to retrieve PCR update counter in the TPM
>     subsystem.
>   - IMA functionality to acquire the update counter from the TPM subsystem.
>   - Measuring the update counter at system boot and at kexec Kernel
>     load.

Then the bugs you mentioned above that may happen between system boot and kexec
load are still going to confuse anyone looking at the log and quote. I don't
think you should mention them. I thought you would provide a way to sync
up on every step...

Also, I thought you had a variable in your kexec series that would prevent all further
logging and measuring once the log had been marshalled during kexec 'exec' stage
and this wasn't necessary.

    Stefan

> 
> 
> This patch series would be a prerequisite for the next version of kexec
> load/execute series[1] and the future IMA log snapshotting patch series.
> 
> [1] https://lore.kernel.org/all/20230703215709.1195644-1-tusharsu@linux.microsoft.com/
>      ima: measure events between kexec load and execute
> 
> [2] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
>      Section 22.4.2, Page 206.
> 
> Tushar Sugandhi (6):
>    tpm: implement TPM2 function to get update counter
>    tpm: provide functionality to get update counter
>    ima: get TPM update counter
>    ima: implement functionality to measure TPM update counter
>    ima: measure TPM update counter at ima_init
>    kexec: measure TPM update counter in ima log at kexec load
> 
>   drivers/char/tpm/tpm-interface.c   | 28 +++++++++++++++++
>   drivers/char/tpm/tpm.h             |  3 ++
>   drivers/char/tpm/tpm2-cmd.c        | 48 ++++++++++++++++++++++++++++++
>   include/linux/ima.h                |  1 +
>   include/linux/tpm.h                |  8 +++++
>   kernel/kexec_file.c                |  3 ++
>   security/integrity/ima/ima.h       |  2 ++
>   security/integrity/ima/ima_init.c  |  3 ++
>   security/integrity/ima/ima_main.c  | 29 ++++++++++++++++++
>   security/integrity/ima/ima_queue.c | 16 ++++++++++
>   10 files changed, 141 insertions(+)
> 

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

On 8/3/23 01:43, Jarkko Sakkinen wrote:
> On Thu Aug 3, 2023 at 12:04 AM EEST, Tushar Sugandhi wrote:
>> Btw, the function tpm2_pcr_read is not exposed directly to the other
>> subsystems (like IMA).  It is exposed via tpm_pcr_read.
>>
>> Do you want to expose tpm2_pcr_read directly,
>> or do you want me to update the function signature of tpm_pcr_read as well?
> As long as you mention that 'update_cnt' causes divegence in the
> in-kernel API, and therefore tpm[12]_pcr_read() cannnot be under the
> same orchestrator.
Yup. I will mention that in the description/comment.
>
> If you take this path, please implement a precursory patch, which
> replace the existing call sites with some sequence of tpm[12]_pcr_read()
> and tpm_is_tpm2() calls.
Sure.  I will add a precursory patch which will replace the existing
call sites.

Thanks for confirming the approach.

~Tushar
> BR, Jarkko

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

On 8/2/23 18:22, Mimi Zohar wrote:
> On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote:
>>  From long description I see zero motivation to ack this change, except
>> some heresay about IMA requiring it. Why does IMA need update_cnt and
>> why this is not documented to the long description?
> The motivation is to detect whether the IMA measurement list has been
> truncated, for whatever reason.  A new IMA record should be defined
> containing the "pcrCounter" value.  (I have not had a chance to review
> this patch set.)
>
> This new record would be a pre-req for both Tushar's "ima: measure
> events between kexec load and execute" patch set and Sush's proposal to
> trim the measurement list.  (I haven't looked at it yet either.)
>
  Thanks Mimi. Take your time.

Re: [PATCH 1/6] tpm: implement TPM2 function to get update counter

[Tushar Sugandhi]

On 8/3/23 01:57, Jarkko Sakkinen wrote:
> On Thu Aug 3, 2023 at 4:22 AM EEST, Mimi Zohar wrote:
>> On Wed, 2023-08-02 at 06:58 +0300, Jarkko Sakkinen wrote:
>>>  From long description I see zero motivation to ack this change, except
>>> some heresay about IMA requiring it. Why does IMA need update_cnt and
>>> why this is not documented to the long description?
>> The motivation is to detect whether the IMA measurement list has been
>> truncated, for whatever reason.  A new IMA record should be defined
>> containing the "pcrCounter" value.  (I have not had a chance to review
>> this patch set.)
>>
>> This new record would be a pre-req for both Tushar's "ima: measure
>> events between kexec load and execute" patch set and Sush's proposal to
>> trim the measurement list.  (I haven't looked at it yet either.)
> Please describe the story in a bit more understandable form. In the
> commit messages it is not good to have some redundancy in patch sets.
>
> BR, Jarkko
Thanks Jarkko.  I had provided the overall context in the cover letter.
But I understand the cover letter will be lost when the patches are
merged. I will describe the story in the patch descriptions as well,
in the next version of this series.

~Tushar

Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter

[Mimi Zohar]

On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
> Currently TPM update counter is not available external to the system,
> for instance, a remote attestation service.  It is a problem because
> the service cannot easily determine if the IMA log entries are missing.
> The IMA functionality needs to be extended to measure the TPM update
> counter from various subsystems in Linux kernel to help detect if
> the IMA log entries are missing.
> 
> Implement a function, 'ima_measure_update_counter()' which would retrieve
> the TPM update counter using the previously defined function
> 'ima_tpm_get_update_counter()'.  Format it as a string with the value 
> "update_counter=<N>;", and measure it using the function
> 'ima_measure_critical_data()'.
> 
> The function takes an event name as input, and the update counter value
> is measured as part of this event.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Explicit TPM2 quote commands do not return the quoted PCR values or the
pcrCounter value.  Define and include a new IMA measurement record
containing the pcrCounter, other TPM info, and IMA info in the IMA
measurement list to help simplify detecting a trimmed/truncated
measurement list.

> ---
>  include/linux/ima.h               |  1 +
>  security/integrity/ima/ima.h      |  1 +
>  security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 86b57757c7b1..f15f3a6a4c72 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label,
>  				     const char *event_name,
>  				     const void *buf, size_t buf_len,
>  				     bool hash, u8 *digest, size_t digest_len);
> +int ima_measure_update_counter(const char *event_name);
>  
>  #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>  extern void ima_appraise_parse_cmdline(void);
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 4acd0e5a830f..5484bd362237 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -168,6 +168,7 @@ int __init ima_init_digests(void);
>  int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>  			  void *lsm_data);
>  int ima_tpm_get_update_counter(u32 *cpu_update_counter);
> +int ima_measure_update_counter(const char *event_name);
>  
>  /*
>   * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d66a0a36415e..1bcd45cc5a6a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
>  }
>  EXPORT_SYMBOL_GPL(ima_measure_critical_data);
>  
> +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
> +int ima_measure_update_counter(const char *event_name)
> +{
> +	int result;
> +	u32 update_counter = 0;
> +	char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
> +	int buf_len;
> +
> +	if (!event_name)
> +		return -ENOPARAM;
> +
> +	result = ima_tpm_get_update_counter(&update_counter);
> +
> +	if (result != 0)
> +		return result;
> +
> +	scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
> +			  update_counter);
> +
> +	buf_len = strlen(buf);
> +
> +	result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
> +				  buf, buf_len, false, NULL, 0);
> 

The new record should contain everything needed to verify the
pcrCounter.  For example, each IMA measurement record updates the
pcrCounter for each TPM bank enabled.  So the number of enabled TPM
banks and number of IMA measurements should also be included in this
record.

Perhaps include a version number as well, so that if we ever want to
include other information, we could.

Mimi


> +
> +	return result;
> +}
> +EXPORT_SYMBOL_GPL(ima_measure_update_counter);
> +
>  static int __init init_ima(void)
>  {
>  	int error;

Re: [PATCH 0/6] Measuring TPM update counter in IMA

[Tushar Sugandhi]

Thanks Stefan for reviewing this series. Appreciate it.

Re-sending this email. I accidentally had some HTML content, the email
bounced back from integrity mailing list.

On 8/3/23 06:37, Stefan Berger wrote:
>
>
> On 8/1/23 14:19, Tushar Sugandhi wrote:
>> Entries in IMA log may be lost due to code bugs, certain error 
>> conditions
>
> I hope we don't have such bugs. And I guess the most critical ones 
> would be
> between logging and PCR extensions
I hope so too, but in general, it’s hard to prove a negative.
That’s why this patch series. :)
>
>
>> being met etc. This can result in TPM PCRs getting out of sync with the
>> IMA log.  One such example is events between kexec 'load' and kexec
>> 'execute' getting lost from the IMA log when the system soft-boots into
>> the new Kernel using kexec[1].  The remote attestation service does not
>
> Though this particular condition I thought would go away with your 
> kexec series.
Currently the events in-between kexec ‘load’ and ‘execute’ are always
lost – because IMA log is captured at ‘load’.  My kexec series addresses
this scenario. But as you said, there is always a possibility that the
events will still be lost during kexec because of error conditions, OOM
etc.
>
> The other conditions would be an out-of-memory or TPM failure. The OOM 
> would
> probably be more critical since something that was supposed to be logged
> couldn't be logged and now you cannot show this anymore and presumably 
> not even
> an error condition could be logged.
>
Precisely. As you can see in patch 5 of this series, I am logging the
counter at ima_init (ima_init_tpm_update_counter).  So we will get the
baseline counter at the system boot, where memory pressure is hopefully
low.  Even if we are not able to log the counter later due to OOM, this
baseline counter along with the number of events in the IMA log should
help detect if IMA log is missing events.
> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179 
>
>
>> have any information if the PCR mismatch with IMA log is because of loss
>> of entries in the IMA log or something else.  TPM 2.0 provides an update
>> counter which is incremented each time a PCR is updated [2]. 
>> Measuring the
>> TPM PCR update counter in IMA subsystem will help the remote attestation
>> service to validate if there are any missing entries in the IMA log, 
>> when
>
>
>
>> the system goes through certain important state changes (e.g. kexec soft
>> boot, IMA log snapshotting etc.)
>>
>> This patch series provides the required functionality to measure the
>> update counter through IMA subsystem by -
>>   - introducing a function to retrieve PCR update counter in the TPM
>>     subsystem.
>>   - IMA functionality to acquire the update counter from the TPM 
>> subsystem.
>>   - Measuring the update counter at system boot and at kexec Kernel
>>     load.
>
> Then the bugs you mentioned above that may happen between system boot 
> and kexec
> load are still going to confuse anyone looking at the log and quote. I 
> don't
> think you should mention them. I thought you would provide a way to sync
I used the kexec load-execute bug as an example to demonstrate the value of
measuring update counter.  There could be other examples which I am not
aware of.  As we discussed above, even when I fix the kexec bug – there is
still a possibility that events may go missing in error/OOM cases.

I can remove the kexec example if it is causing confusion.
Please let me know.
> up on every step...
I don’t fully understand what you mean by “provide a way to sync up
on every step”.  Could you please elaborate?
>
> Also, I thought you had a variable in your kexec series that would 
> prevent all further
> logging and measuring once the log had been marshalled during kexec 
> 'exec' stage
> and this wasn't necessary.
>
No, the variable suspend_ima_measurements[1] suspends the measurements
while copying the kexec buffer during kexec execute to ensure consistency
of the IMA measurements.  It doesn’t prevent all future logging. The
variable is reset and the IMA measurements resume when the system boots
into the new Kernel image.

[1] 
https://patchwork.kernel.org/project/linux-integrity/patch/20230703215709.1195644-10-tusharsu@linux.microsoft.com/

~Tushar
>    Stefan
>
>>
>>
>> This patch series would be a prerequisite for the next version of kexec
>> load/execute series[1] and the future IMA log snapshotting patch series.
>>
>> [1] 
>> https://lore.kernel.org/all/20230703215709.1195644-1-tusharsu@linux.microsoft.com/
>>      ima: measure events between kexec load and execute
>>
>> [2] 
>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
>>      Section 22.4.2, Page 206.
>>
>> Tushar Sugandhi (6):
>>    tpm: implement TPM2 function to get update counter
>>    tpm: provide functionality to get update counter
>>    ima: get TPM update counter
>>    ima: implement functionality to measure TPM update counter
>>    ima: measure TPM update counter at ima_init
>>    kexec: measure TPM update counter in ima log at kexec load
>>
>>   drivers/char/tpm/tpm-interface.c   | 28 +++++++++++++++++
>>   drivers/char/tpm/tpm.h             |  3 ++
>>   drivers/char/tpm/tpm2-cmd.c        | 48 ++++++++++++++++++++++++++++++
>>   include/linux/ima.h                |  1 +
>>   include/linux/tpm.h                |  8 +++++
>>   kernel/kexec_file.c                |  3 ++
>>   security/integrity/ima/ima.h       |  2 ++
>>   security/integrity/ima/ima_init.c  |  3 ++
>>   security/integrity/ima/ima_main.c  | 29 ++++++++++++++++++
>>   security/integrity/ima/ima_queue.c | 16 ++++++++++
>>   10 files changed, 141 insertions(+)
>>

Re: [PATCH 0/6] Measuring TPM update counter in IMA

[Stefan Berger]

On 8/3/23 17:30, Tushar Sugandhi wrote:

> 
> Thanks Stefan for reviewing this series. Appreciate it.
> 
> On 8/3/23 06:37, Stefan Berger wrote:
>>
>>
>> On 8/1/23 14:19, Tushar Sugandhi wrote:
>>> Entries in IMA log may be lost due to code bugs, certain error conditions
>>
>> I hope we don't have such bugs. And I guess the most critical ones would be
>> between logging and PCR extensions
>>
> I hope so too, but in general, it’s hard to prove a negative.
> That’s why this patch series. :)
>>> being met etc.  This can result in TPM PCRs getting out of sync with the
>>> IMA log.  One such example is events between kexec 'load' and kexec
>>> 'execute' getting lost from the IMA log when the system soft-boots into
>>> the new Kernel using kexec[1].  The remote attestation service does not
>>
>> Though this particular condition I thought would go away with your kexec series.
>>
> Currently the events in-between kexec ‘load’ and ‘execute’ are always
> lost – because IMA log is captured at ‘load’.  My kexec series addresses
> this scenario. But as you said, there is always a possibility that the
> events will still be lost during kexec because of error conditions, OOM
> etc.
>> The other conditions would be an out-of-memory or TPM failure. The OOM would
>> probably be more critical since something that was supposed to be logged
>> couldn't be logged and now you cannot show this anymore and presumably not even
>> an error condition could be logged.
>>
> Precisely. As you can see in patch 5 of this series, I am logging the
> counter at ima_init (ima_init_tpm_update_counter).  So we will get the
> baseline counter at the system boot, where memory pressure is hopefully
> low.  Even if we are not able to log the counter later due to OOM, this
> baseline counter along with the number of events in the IMA log should
> help detect if IMA log is missing events.

How do you account for updates to other PCRs than PCR 10 that a user may use for whatever reason?
I think you would have to have the TPM driver count the updates for PCR 10.
Form what I can see there's one global PCR update counter for all PCRs and
all banks.

Also, if we hit an oom condition when logging then the PCR is not extended, which
is good since otherwise we would be hopelessly out-of-sync.


> 
>> https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_queue.c#L179
>>
>>> have any information if the PCR mismatch with IMA log is because of loss
>>> of entries in the IMA log or something else.  TPM 2.0 provides an update
>>> counter which is incremented each time a PCR is updated [2]. Measuring the
>>> TPM PCR update counter in IMA subsystem will help the remote attestation
>>> service to validate if there are any missing entries in the IMA log, when
>>
>>
>>
>>> the system goes through certain important state changes (e.g. kexec soft
>>> boot, IMA log snapshotting etc.)
>>>
>>> This patch series provides the required functionality to measure the
>>> update counter through IMA subsystem by -
>>>   - introducing a function to retrieve PCR update counter in the TPM
>>>     subsystem.
>>>   - IMA functionality to acquire the update counter from the TPM subsystem.
>>>   - Measuring the update counter at system boot and at kexec Kernel
>>>     load.
>>
>> Then the bugs you mentioned above that may happen between system boot and kexec
>> load are still going to confuse anyone looking at the log and quote. I don't
>> think you should mention them. I thought you would provide a way to sync
> I used the kexec load-execute bug as an example to demonstrate the value of
> measuring update counter.  There could be other examples which I am not
> aware of.  As we discussed above, even when I fix the kexec bug – there is
> still a possibility that events may go missing in error/OOM cases.

Yes, and we're not extending the PCRs then either, which would be catastrophic
if we were.

> 
> I can remove the kexec example if it is causing confusion.> Please let me know.

I am not convinced we need this series  ... :-( Your kexec series prevents
further logging and especially PCR extensions after the frozen measurement log
has been created and in ima_add_template_entry(), if we hit an oom condition,
then we luckily do not extend the PCR either. If either the log was to have one
more entry than number PCR extensions occurred or vice versa, then the remote
attestation service will see this mismatch no matter what and all the PCR update
counter won't help (and is generally not a good indicator for this purpose imo)
for it to recover from this. It's better to declare the system as un-trusted/
corrupted in this case then.

    Stefan


>> up on every step...
>>
> I don’t fully understand what you mean by “provide a way to sync up
> on every step”.  Could you please elaborate?
> 
>> Also, I thought you had a variable in your kexec series that would prevent all further
>> logging and measuring once the log had been marshalled during kexec 'exec' stage
>> and this wasn't necessary.
>>
> No, the variable suspend_ima_measurements[1] suspends the measurements
> while copying the kexec buffer during kexec execute to ensure consistency
> of the IMA measurements.  It doesn’t prevent all future logging. The
> variable is reset and the IMA measurements resume when the system boots
> into the new Kernel image.
> 
> [1] https://patchwork.kernel.org/project/linux-integrity/patch/20230703215709.1195644-10-tusharsu@linux.microsoft.com/
> 
> ~Tushar
> 
>> Stefan
>>
>>>
>>>
>>> This patch series would be a prerequisite for the next version of kexec
>>> load/execute series[1] and the future IMA log snapshotting patch series.
>>>
>>> [1] https://lore.kernel.org/all/20230703215709.1195644-1-tusharsu@linux.microsoft.com/
>>>      ima: measure events between kexec load and execute
>>>
>>> [2] https://trustedcomputinggroup.org/wp-content/uploads/TCG_TPM2_r1p59_Part3_Commands_pub.pdf
>>>      Section 22.4.2, Page 206.
>>>
>>> Tushar Sugandhi (6):
>>>    tpm: implement TPM2 function to get update counter
>>>    tpm: provide functionality to get update counter
>>>    ima: get TPM update counter
>>>    ima: implement functionality to measure TPM update counter
>>>    ima: measure TPM update counter at ima_init
>>>    kexec: measure TPM update counter in ima log at kexec load
>>>
>>>   drivers/char/tpm/tpm-interface.c   | 28 +++++++++++++++++
>>>   drivers/char/tpm/tpm.h             |  3 ++
>>>   drivers/char/tpm/tpm2-cmd.c        | 48 ++++++++++++++++++++++++++++++
>>>   include/linux/ima.h                |  1 +
>>>   include/linux/tpm.h                |  8 +++++
>>>   kernel/kexec_file.c                |  3 ++
>>>   security/integrity/ima/ima.h       |  2 ++
>>>   security/integrity/ima/ima_init.c  |  3 ++
>>>   security/integrity/ima/ima_main.c  | 29 ++++++++++++++++++
>>>   security/integrity/ima/ima_queue.c | 16 ++++++++++
>>>   10 files changed, 141 insertions(+)
>>>

Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

[Mimi Zohar]

On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
> IMA log entries can be lost due to a variety of causes, such as code bugs
> or error conditions, leading to a mismatch between TPM PCRs and
> the IMA log.  Measuring TPM PCR update counter during ima_init would
> provide a baseline counter for the number of times the TPM PCRs are
> updated.  The remote attestation service can compare this baseline
> counter with a subsequent measured one (e.g., post-kexec soft-boot) to
> identify if there are any lost IMA log events.
> 
> Measure the TPM update counter at ima init.

No need for separate patches for one line changes like this.  Either
merge patches 5/6 and 6/6 or all three 4/6, 5/6, 6/6 together.

> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_init.c | 3 +++
>  security/integrity/ima/ima_main.c | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 63979aefc95f..9bb18d6c2fd6 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -154,5 +154,8 @@ int __init ima_init(void)
>  				  UTS_RELEASE, strlen(UTS_RELEASE), false,
>  				  NULL, 0);
>  
> +	/* Measures TPM update counter at ima_init */
> +	ima_measure_update_counter("ima_init_tpm_update_counter");
> +

With "ima_policy=critical_data" on the boot command line, the IMA
measurement list record looks like:

6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b

Please change the "ima_init_tpm_update_counter" to something shorter
and the hex encoded ascii string and pcr counter to something readable.
Perhaps name this critical-data "tpm" and "tpm-info", similar to the
SELinux "selinux" and "selinux-state".  Then again, if this is TPM
critical-data we should rethink what other info should be included.

>  	return rc;
>  }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1bcd45cc5a6a..93357c245e82 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>  				   buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
>  				   NULL, false, NULL, 0);
>  	fdput(f);
> +
>  }
>  
>  /**

Unnecessary change.

-- 
thanks,

Mimi

Re: [PATCH 0/6] Measuring TPM update counter in IMA

[Mimi Zohar]

On Thu, 2023-08-03 at 18:09 -0400, Stefan Berger wrote:
> > I can remove the kexec example if it is causing confusion.> Please let me know.
> 
> I am not convinced we need this series  ... :-( Your kexec series prevents
> further logging and especially PCR extensions after the frozen measurement log
> has been created and in ima_add_template_entry(), if we hit an oom condition,
> then we luckily do not extend the PCR either. If either the log was to have one
> more entry than number PCR extensions occurred or vice versa, then the remote
> attestation service will see this mismatch no matter what and all the PCR update
> counter won't help (and is generally not a good indicator for this purpose imo)
> for it to recover from this. It's better to declare the system as un-trusted/
> corrupted in this case then.

As previously mentioned, there is a patch set that doesn't carry any
records across kexec, if the the measurement list is too large, and
another proposal to trim the measurement list.

In both of these cases including a new IMA mesaurement record, at least
after the boot_aggregate, would help simplify detecting whether the
measurement list has been trimmed/truncated.

-- 
thanks,

Mimi

Re: [PATCH 0/6] Measuring TPM update counter in IMA

[Stefan Berger]

On 8/3/23 18:36, Mimi Zohar wrote:
> On Thu, 2023-08-03 at 18:09 -0400, Stefan Berger wrote:
>>> I can remove the kexec example if it is causing confusion.> Please let me know.
>>
>> I am not convinced we need this series  ... :-( Your kexec series prevents
>> further logging and especially PCR extensions after the frozen measurement log
>> has been created and in ima_add_template_entry(), if we hit an oom condition,
>> then we luckily do not extend the PCR either. If either the log was to have one
>> more entry than number PCR extensions occurred or vice versa, then the remote
>> attestation service will see this mismatch no matter what and all the PCR update
>> counter won't help (and is generally not a good indicator for this purpose imo)
>> for it to recover from this. It's better to declare the system as un-trusted/
>> corrupted in this case then.
> 
> As previously mentioned, there is a patch set that doesn't carry any
> records across kexec, if the the measurement list is too large, and
> another proposal to trim the measurement list.
> 
> In both of these cases including a new IMA mesaurement record, at least
> after the boot_aggregate, would help simplify detecting whether the
> measurement list has been trimmed/truncated.
> 

And if you can detect that I would log an event but not using the PCR update counter.
Unless the state of PCRs is also logged, it's going to be unrecoverable for a log+quote
verifier from there.

    Stefan

Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter

[Tushar Sugandhi]

Thanks for the review Mimi.

On 8/3/23 14:42, Mimi Zohar wrote:
> On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
>> Currently TPM update counter is not available external to the system,
>> for instance, a remote attestation service.  It is a problem because
>> the service cannot easily determine if the IMA log entries are missing.
>> The IMA functionality needs to be extended to measure the TPM update
>> counter from various subsystems in Linux kernel to help detect if
>> the IMA log entries are missing.
>>
>> Implement a function, 'ima_measure_update_counter()' which would retrieve
>> the TPM update counter using the previously defined function
>> 'ima_tpm_get_update_counter()'.  Format it as a string with the value
>> "update_counter=<N>;", and measure it using the function
>> 'ima_measure_critical_data()'.
>>
>> The function takes an event name as input, and the update counter value
>> is measured as part of this event.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> Explicit TPM2 quote commands do not return the quoted PCR values or the
> pcrCounter value.  Define and include a new IMA measurement record
> containing the pcrCounter, other TPM info, and IMA info in the IMA
> measurement list to help simplify detecting a trimmed/truncated
> measurement list.
Sounds good.
>> ---
>>   include/linux/ima.h               |  1 +
>>   security/integrity/ima/ima.h      |  1 +
>>   security/integrity/ima/ima_main.c | 28 ++++++++++++++++++++++++++++
>>   3 files changed, 30 insertions(+)
>>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index 86b57757c7b1..f15f3a6a4c72 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -40,6 +40,7 @@ extern int ima_measure_critical_data(const char *event_label,
>>   				     const char *event_name,
>>   				     const void *buf, size_t buf_len,
>>   				     bool hash, u8 *digest, size_t digest_len);
>> +int ima_measure_update_counter(const char *event_name);
>>   
>>   #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
>>   extern void ima_appraise_parse_cmdline(void);
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 4acd0e5a830f..5484bd362237 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -168,6 +168,7 @@ int __init ima_init_digests(void);
>>   int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>>   			  void *lsm_data);
>>   int ima_tpm_get_update_counter(u32 *cpu_update_counter);
>> +int ima_measure_update_counter(const char *event_name);
>>   
>>   /*
>>    * used to protect h_table and sha_table
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index d66a0a36415e..1bcd45cc5a6a 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -1071,6 +1071,34 @@ int ima_measure_critical_data(const char *event_label,
>>   }
>>   EXPORT_SYMBOL_GPL(ima_measure_critical_data);
>>   
>> +#define IMA_TPM_UPDATE_CTR_BUF_SIZE 128
>> +int ima_measure_update_counter(const char *event_name)
>> +{
>> +	int result;
>> +	u32 update_counter = 0;
>> +	char buf[IMA_TPM_UPDATE_CTR_BUF_SIZE];
>> +	int buf_len;
>> +
>> +	if (!event_name)
>> +		return -ENOPARAM;
>> +
>> +	result = ima_tpm_get_update_counter(&update_counter);
>> +
>> +	if (result != 0)
>> +		return result;
>> +
>> +	scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
>> +			  update_counter);
>> +
>> +	buf_len = strlen(buf);
>> +
>> +	result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
>> +				  buf, buf_len, false, NULL, 0);
>>
> The new record should contain everything needed to verify the
> pcrCounter.  For example, each IMA measurement record updates the
> pcrCounter for each TPM bank enabled.  So the number of enabled TPM
> banks and number of IMA measurements should also be included in this
> record.
Agreed. That should be valuable information.
How does the below format look like for the buf above?

version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;

>
> Perhaps include a version number as well, so that if we ever want to
> include other information, we could.
By version number, do you mean kernel_version, or a new version
number specific to this record? Or something else?

~Tushar
> Mimi
>
>
>> +
>> +	return result;
>> +}
>> +EXPORT_SYMBOL_GPL(ima_measure_update_counter);
>> +
>>   static int __init init_ima(void)
>>   {
>>   	int error;

Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

[Tushar Sugandhi]

On 8/3/23 15:15, Mimi Zohar wrote:
> On Tue, 2023-08-01 at 11:19 -0700, Tushar Sugandhi wrote:
>> IMA log entries can be lost due to a variety of causes, such as code bugs
>> or error conditions, leading to a mismatch between TPM PCRs and
>> the IMA log.  Measuring TPM PCR update counter during ima_init would
>> provide a baseline counter for the number of times the TPM PCRs are
>> updated.  The remote attestation service can compare this baseline
>> counter with a subsequent measured one (e.g., post-kexec soft-boot) to
>> identify if there are any lost IMA log events.
>>
>> Measure the TPM update counter at ima init.
> No need for separate patches for one line changes like this.  Either
> merge patches 5/6 and 6/6 or all three 4/6, 5/6, 6/6 together.
>
Sounds good.
I will merge 4/6, 5/6, 6/6 together.
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
>> ---
>>   security/integrity/ima/ima_init.c | 3 +++
>>   security/integrity/ima/ima_main.c | 1 +
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
>> index 63979aefc95f..9bb18d6c2fd6 100644
>> --- a/security/integrity/ima/ima_init.c
>> +++ b/security/integrity/ima/ima_init.c
>> @@ -154,5 +154,8 @@ int __init ima_init(void)
>>   				  UTS_RELEASE, strlen(UTS_RELEASE), false,
>>   				  NULL, 0);
>>   
>> +	/* Measures TPM update counter at ima_init */
>> +	ima_measure_update_counter("ima_init_tpm_update_counter");
>> +
> With "ima_policy=critical_data" on the boot command line, the IMA
> measurement list record looks like:
>
> 6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b
>
> Please change the "ima_init_tpm_update_counter" to something shorter
> and the hex encoded ascii string and pcr counter to something readable.
I believe you are seeing the above line in ascill_runtime_measurements log.

The ascii logging format is consistent with other event data for 
critical_data event e.g. kernel_version.
10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf 
sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version 
362e332e302d7263332b
10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf 
sha1:d8c187524412f74a961f2051a9529c009e700337 
ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b

Entries in the binary runtime measurements look readable to me.

ima_init_tpm_update_counter update_counter=130;
...
kexec_load_tpm_update_counte rupdate_counter=133;

Please let me know if you still want me to change the format.

> Perhaps name this critical-data "tpm" and "tpm-info", similar to the
 From patch 4/6:
+    result = ima_measure_critical_data("tpm_pcr_update_counter", 
event_name,
+                  buf, buf_len, false, NULL, 0);

The critical_data event_label value is currently set to 
"tpm_pcr_update_counter".
I can rename event_label to "tpm-info", so that the admins can filter the
event in IMA policy based on the label if needed.

As you know, event_label doesn't appear in IMA log, it can appear in IMA 
policy.
Whereas event_name appears in IMA log.

I was thinking of using event_name to identify when was the info captured.
(e.g. ima_init, kexec_load, or at some other event in future).

We can either do
(a)
event_label = "tpm-info" event_name = "tpm-info-ima-init" | 
"tpm-info-kexec-load" | ...

-or-

(b)
event_label = "tpm" event_name = "tpm-info"
and event_data to describe the where/when this info was captured.
e.g.
version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;event=kexec_load;

Let me know if you would prefer option (a) or (b) or something else.


> SELinux "selinux" and "selinux-state".  Then again, if this is TPM
> critical-data we should rethink what other info should be included.
As you suggested in Patch 4/6, I will add version, number of enabled pcr 
banks,
pcrUpdateCounter, and num_ima_measurements. I think we should include 
the TPM
version as well (1 v/s 2).

Please let me know if you think of any other attribute to record.

>>   	return rc;
>>   }
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 1bcd45cc5a6a..93357c245e82 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -1035,6 +1035,7 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>>   				   buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
>>   				   NULL, false, NULL, 0);
>>   	fdput(f);
>> +
>>   }
>>   
>>   /**
> Unnecessary change.
>
oops. Thanks for catching. Will fix.


~Tushar

Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

[Mimi Zohar]

On Thu, 2023-08-03 at 16:34 -0700, Tushar Sugandhi wrote:

> >> +++ b/security/integrity/ima/ima_init.c
> >> @@ -154,5 +154,8 @@ int __init ima_init(void)
> >>   				  UTS_RELEASE, strlen(UTS_RELEASE), false,
> >>   				  NULL, 0);
> >>   
> >> +	/* Measures TPM update counter at ima_init */
> >> +	ima_measure_update_counter("ima_init_tpm_update_counter");
> >> +
> > With "ima_policy=critical_data" on the boot command line, the IMA
> > measurement list record looks like:
> >
> > 6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b
> >
> > Please change the "ima_init_tpm_update_counter" to something shorter
> > and the hex encoded ascii string and pcr counter to something readable.
> I believe you are seeing the above line in ascill_runtime_measurements log.

Yes, the ascii_runtime_measurements are suppose to be readable to the
end user.

> The ascii logging format is consistent with other event data for 
> critical_data event e.g. kernel_version.

Then you got it wrong.

> 10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf 
> sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version 
> 362e332e302d7263332b
> 10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf 
> sha1:d8c187524412f74a961f2051a9529c009e700337 
> ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
> 
> Entries in the binary runtime measurements look readable to me.

You've inverted the meaning of the ascii and binary runtime measurement
lists.  For comparison look at the ima-ng/ima-sig records.

> ima_init_tpm_update_counter update_counter=130;
> ...
> kexec_load_tpm_update_counte rupdate_counter=133;
> 
> Please let me know if you still want me to change the format.

OI course the ascii measurement list should be human readable.

> > Perhaps name this critical-data "tpm" and "tpm-info", similar to the
>  From patch 4/6:
> +    result = ima_measure_critical_data("tpm_pcr_update_counter", 
> event_name,
> +                  buf, buf_len, false, NULL, 0);
> 
> The critical_data event_label value is currently set to 
> "tpm_pcr_update_counter".

Why is the string so long?   Long strings or variables don't make the
code or logs more understandable.  Please shorten both the strings and
variables.

> I can rename event_label to "tpm-info", so that the admins can filter the
> event in IMA policy based on the label if needed.

The new record needs to be self containd and verifiable.  The
additional info I suggested were just examples.  Please take the time
to consider what needs to be included in the new record.  Decide
whether this is a TPM security critical data record.  Only then, decide
on the naming.

-- 
thanks,

Mimi

Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter

[Mimi Zohar]

On Thu, 2023-08-03 at 16:01 -0700, Tushar Sugandhi wrote:
> >> +    scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
> >> +                      update_counter);
> >> +
> >> +    buf_len = strlen(buf);
> >> +
> >> +    result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
> >> +                              buf, buf_len, false, NULL, 0);
> >>
> > The new record should contain everything needed to verify the
> > pcrCounter.  For example, each IMA measurement record updates the
> > pcrCounter for each TPM bank enabled.  So the number of enabled TPM
> > banks and number of IMA measurements should also be included in this
> > record.
> Agreed. That should be valuable information.
> How does the below format look like for the buf above?
> 
> version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;

Refer to comment in 5/6.

> > Perhaps include a version number as well, so that if we ever want to
> > include other information, we could.
> By version number, do you mean kernel_version, or a new version
> number specific to this record? Or something else?

This is a record version type number.  The record format shouldn't
change, but we should be prepared for it changing.  A single number
should be fine.

-- 
thanks,

Mimi

Re: [PATCH 5/6] ima: measure TPM update counter at ima_init

[Tushar Sugandhi]

On 8/3/23 18:18, Mimi Zohar wrote:
> On Thu, 2023-08-03 at 16:34 -0700, Tushar Sugandhi wrote:
>
>>>> +++ b/security/integrity/ima/ima_init.c
>>>> @@ -154,5 +154,8 @@ int __init ima_init(void)
>>>>    				  UTS_RELEASE, strlen(UTS_RELEASE), false,
>>>>    				  NULL, 0);
>>>>    
>>>> +	/* Measures TPM update counter at ima_init */
>>>> +	ima_measure_update_counter("ima_init_tpm_update_counter");
>>>> +
>>> With "ima_policy=critical_data" on the boot command line, the IMA
>>> measurement list record looks like:
>>>
>>> 6e190cc643ff0b718485966a0300473baedface735 ima_init_tpm_update_counter 7570646174655f636f756e7465723d3330383b
>>>
>>> Please change the "ima_init_tpm_update_counter" to something shorter
>>> and the hex encoded ascii string and pcr counter to something readable.
>> I believe you are seeing the above line in ascill_runtime_measurements log.
> Yes, the ascii_runtime_measurements are suppose to be readable to the
> end user.
We were passing string literals to 'buf' param in 
ima_measure_critical_data().
I believe we need to convert them first.

>> The ascii logging format is consistent with other event data for
>> critical_data event e.g. kernel_version.
> Then you got it wrong.
I see.  I will fix it for tpm in this patch series.
I think I should spin up another series to fix it for
selinux, kernel info, DM etc.
>> 10 8f449175bbf88bc55fc1127466628c39a3957d15 ima-buf
>> sha1:4acab4fbb08db663b7b7b4528e8729187d726782 kernel_version
>> 362e332e302d7263332b
>> 10 f10678b63c4b2529339dff02282e63d9c6bb0385 ima-buf
>> sha1:d8c187524412f74a961f2051a9529c009e700337
>> ima_init_tpm_update_counter 7570646174655f636f756e7465723d3133303b
>>
>> Entries in the binary runtime measurements look readable to me.
> You've inverted the meaning of the ascii and binary runtime measurement
> lists.  For comparison look at the ima-ng/ima-sig records.
Yup.  Agreed.
>> ima_init_tpm_update_counter update_counter=130;
>> ...
>> kexec_load_tpm_update_counte rupdate_counter=133;
>>
>> Please let me know if you still want me to change the format.
> OI course the ascii measurement list should be human readable.
Yup.  I will make the changes as I mentioned above.
>
>>> Perhaps name this critical-data "tpm" and "tpm-info", similar to the
>>   From patch 4/6:
>> +    result = ima_measure_critical_data("tpm_pcr_update_counter",
>> event_name,
>> +                  buf, buf_len, false, NULL, 0);
>>
>> The critical_data event_label value is currently set to
>> "tpm_pcr_update_counter".
> Why is the string so long?   Long strings or variables don't make the
> code or logs more understandable.  Please shorten both the strings and
> variables.
Agreed.  I will name this "tpm" and "tpm-info" or something similar.
>> I can rename event_label to "tpm-info", so that the admins can filter the
>> event in IMA policy based on the label if needed.
> The new record needs to be self containd and verifiable.  The
> additional info I suggested were just examples.  Please take the time
> to consider what needs to be included in the new record.  Decide
> whether this is a TPM security critical data record.  Only then, decide
> on the naming.
Yes.  I will explore what other potential attributes can be added to 
this record.
And I'll share them here for the community feedback.

Re: [PATCH 4/6] ima: implement functionality to measure TPM update counter

[Tushar Sugandhi]

On 8/3/23 18:22, Mimi Zohar wrote:
> On Thu, 2023-08-03 at 16:01 -0700, Tushar Sugandhi wrote:
>>>> +    scnprintf(buf, IMA_TPM_UPDATE_CTR_BUF_SIZE, "update_counter=%u;",
>>>> +                      update_counter);
>>>> +
>>>> +    buf_len = strlen(buf);
>>>> +
>>>> +    result = ima_measure_critical_data("tpm_pcr_update_counter", event_name,
>>>> +                              buf, buf_len, false, NULL, 0);
>>>>
>>> The new record should contain everything needed to verify the
>>> pcrCounter.  For example, each IMA measurement record updates the
>>> pcrCounter for each TPM bank enabled.  So the number of enabled TPM
>>> banks and number of IMA measurements should also be included in this
>>> record.
>> Agreed. That should be valuable information.
>> How does the below format look like for the buf above?
>>
>> version=<N>.<N>.<N>;num_enabled_pcr_banks=<N>;pcrUpdateCounter=<N>;num_ima_measurements=<N>;
> Refer to comment in 5/6.
Responded.
>>> Perhaps include a version number as well, so that if we ever want to
>>> include other information, we could.
>> By version number, do you mean kernel_version, or a new version
>> number specific to this record? Or something else?
> This is a record version type number.  The record format shouldn't
> change, but we should be prepared for it changing.  A single number
> should be fine.
>
Sounds good.