[RFC PATCH v2 00/13] ima: get rid of hard dependency on SHA-1

[RFC PATCH v2 00/13] ima: get rid of hard dependency on SHA-1

[Nicolai Stange]

Hi,

this is v2 of the RFC series to disentangle IMA from its current
dependency on a working SHA-1 implementation.

For reference, v1 can be found at [1].

Several options for when and how to invalidate unsupported TPM PCR banks
by extending them with a unique constant had been discussed at the v1:
a.) every single time a new entry gets added to the measurement list
b.) or only once.

b.) is appealing, because it enables recognizing unsupported banks right
away from their value, but comes at a significant additional complexity.
Fortunately, it turned out that it's possible to develop b.) incrementally
on top of a.), so this series can get truncated
- after [5/13] ("ima: select CRYPTO_SHA256 from Kconfig") to get a.),
- or after [9/13] ("ima: invalidate unsupported PCR banks only once")
  to get a partial b.), invalidating unsupported banks only once for
  each kernel booted, but redoing it for each kernel in a kexec chain,
- or not at all to get the full b.), i.e. to skip reinvalidations even
  from later kernels in the kexec chain if possible.

I would personally go for the full set, because it also enables some
perhaps helpful diagnostics for the kernel log, but OTOH I'm clearly
biased now because I've implemented everthing. So it's your judgement
call now on how to proceed. Either way, I would send the next iteration in
non-RFC mode with the full CC set. If you opted for a.) only, it would be
a.) only, i.e. [1-5/13]. If you decided for b.), it might make sense to
send in two batches to facilitate review: [1-9/13] first and the rest
somewhen later.

FWIW, I did some testing now, on the full series in a VM with a swtpm
attached to it:
- both with and without CONFIG_TCG_TPM2_HMAC (for [10/13] ("tpm:
  authenticate tpm2_pcr_read()" coverage) and
- with a focus on verifying everything related to the new invalidation
  logic is working as intended.

Thanks a lot!

Nicolai



Changes to v1:
- [v1 1/7] ("ima: don't expose runtime_measurements for unsupported
  hashes"): no change.
- [v1 2/7] ("ima: always create runtime_measurements sysfs file for
  ima_hash"): no change.
- [v1 3/7] ("ima: move INVALID_PCR() to ima.h"): moved to [v2 6/13],
  otherwise no change.
- [v1 4/7] ("ima: track the set of PCRs ever extended"):
  moved to [v2 8/13], drop code restoring ima_extended_pcrs_mask at kexec,
  update it from ima_pcr_extend() only if the tpm_pcr_extend() was
  successful.
- [v1 5/7] ("tpm: enable bank selection for PCR extend"): moved to
  [v2 7/13], fix a bug by actually passing the skip mask from
  tpm_pcr_extend() to tpm2_pcr_extend().
- [v1 6/7] ("ima: invalidate unsupported PCR banks once at first use"):
  gone, superseded by the new
    [v2 3/13]  ("invalidate unsupported PCR banks")
    [v2 9/13]  ("ima: invalidate unsupported PCR banks only once")
    [v2 13/13] ("ima: don't re-invalidate unsupported PCR banks after
              kexec")
- [v1 7/7] ("ima: make SHA1 non-mandatory"): moved to [v2 4/13],
  diff context updates due to ima_unsupported_tpm_banks_mask not
  existing yet at this point in the series.

- [v2 5/13] ("ima: select CRYPTO_SHA256 from Kconfig"): new to
  (hopefully) address feedback at [2].
- [v2 10/13] ("tpm: authenticate tpm2_pcr_read()"): new, prerequisite
  for the next in a sense.
- [v2 11/13] ("ima: introduce ima_pcr_invalidated_banks() helper"): new,
  prerequisite for [13/13].
- [v2 12/13] ("ma: make ima_free_tfm()'s linkage extern"): new,
  likewise a prerequisite for [13/13].


[1] https://lore.kernel.org/r/20250313173339.3815589-1-nstange@suse.de
[2] https://lore.kernel.org/r/4e760360258bda56fbcb8f67e865a7a4574c305a.camel@linux.ibm.com


Nicolai Stange (13):
  ima: don't expose runtime_measurements for unsupported hashes
  ima: always create runtime_measurements sysfs file for ima_hash
  ima: invalidate unsupported PCR banks
  ima: make SHA1 non-mandatory
  ima: select CRYPTO_SHA256 from Kconfig
  ima: move INVALID_PCR() to ima.h
  tpm: enable bank selection for PCR extend
  ima: track the set of PCRs ever extended
  ima: invalidate unsupported PCR banks only once
  tpm: authenticate tpm2_pcr_read()
  ima: introduce ima_pcr_invalidated_banks() helper
  ima: make ima_free_tfm()'s linkage extern
  ima: don't re-invalidate unsupported PCR banks after kexec

 drivers/char/tpm/tpm-interface.c      |  29 +++-
 drivers/char/tpm/tpm.h                |   3 +-
 drivers/char/tpm/tpm2-cmd.c           |  75 ++++++++-
 include/linux/tpm.h                   |   3 +
 security/integrity/ima/Kconfig        |  15 ++
 security/integrity/ima/ima.h          |  12 ++
 security/integrity/ima/ima_crypto.c   | 216 ++++++++++++++++++++++----
 security/integrity/ima/ima_fs.c       |  41 +++--
 security/integrity/ima/ima_policy.c   |   5 +-
 security/integrity/ima/ima_queue.c    |  54 ++++++-
 security/integrity/ima/ima_template.c |  84 +++++++++-
 11 files changed, 471 insertions(+), 66 deletions(-)

-- 
2.49.0

[RFC PATCH v2 01/13] ima: don't expose runtime_measurements for unsupported hashes

[Nicolai Stange]

IMA creates one runtime_measurements_<hash-algo> sysfs file for every TPM
bank + for SHA1 if not covered by any such. These differ only in that the
template hash value for each record is of the file's associated algorithm
each.

The kernel does not necessarily support each hash algorithm associated
with some TPM bank though -- the most common case probably being that the
algorithm is not built-in, but provided as a module, if at all, and thus
not available at IMA init time yet.

If that happens to be the case, the behavior is a bit counter-intuitive:
probably for historic reasons and to still extend the TPM bank with
something, a record's template hash is filled with the padded SHA1 value.
That is, it is perfectly possible that runtime_measurements_sha256 contains
padded SHA1 template hashes if SHA-256 was unavailable at IMA init.

I would argue that it's likely that no existing userspace tool is relying
on this fallback logic -- they either wouldn't consume the hash value from
the measurement list directly but recreate it by themselves, as is required
for verification against PCRs, or, if they did, they would somehow assume a
hash algorithm and expect the hashes in the measurement list to be of that
type. If of the latter kind, this could even lead to hard to debug
verification failures. For example, from looking at keylime's current
code, the verifier logic seems to assume that the template hashes found
in the provided measurement list are of the configured 'ima_log_hash_alg'
type. In particular, it does not check against padded SHA1 upon
mismatch.

That being said, there's also another dimension: currently IMA has a
hard requirement on SHA-1 and subsequent patches in this series will
attempt to get rid of that. If SHA-1 is not available at IMA init though,
it would also mean that padded SHA-1 values cannot get filled in as a
fallback for other unsupported algorithms. Substituting something like
hard coded all-zeroes or all-ones would be dangerous, because some
application or user scripts could perhaps (ab)use the template hashes from
the exported measurement lists for some kind of fingerprinting scheme or
so.

In conclusion, I think it's best to not create the
runtime_measurements_<hash-algo> sysfs files for hash algorithms not
supported by the kernel. That way, applications expecting a certain
hash algorithm for the measurement list and which are not able to handle
the padded-SHA1 fallback scheme would fail with a clear indication on what
the problem is. Furthermore, as digests for unsupported banks are not
getting exposed to userspace anymore, we'll have all flexibility to
set it to any value internally, including all-ones as will be needed in
a subsequent patch when addressing PCR extend for unsupported banks.

So, do not create runtime_measurements_<hash-algo> sysfs files for
unsupported hash algorithms. Likewise for their ascii counterparts.

Note that at this point, SHA-1 is still mandatory, and thus,
runtime_measurements_sha1 as well as the "runtime_measurements" will
remain there, even though the code has provisions already to skip their
creation as well in case SHA-1 was unavailable.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima_fs.c | 35 +++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e4a79a9b2d58..a8df2fe5f4cb 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -454,6 +454,9 @@ static int __init create_securityfs_measurement_lists(void)
 		return -ENOMEM;
 
 	for (i = 0; i < securityfs_measurement_list_count; i++) {
+		if (!ima_algo_array[i].tfm)
+			continue;
+
 		algo = ima_algo_array[i].algo;
 
 		sprintf(file_name, "ascii_runtime_measurements_%s",
@@ -573,20 +576,26 @@ int __init ima_fs_init(void)
 	if (ret != 0)
 		goto out;
 
-	binary_runtime_measurements =
-	    securityfs_create_symlink("binary_runtime_measurements", ima_dir,
-				      "binary_runtime_measurements_sha1", NULL);
-	if (IS_ERR(binary_runtime_measurements)) {
-		ret = PTR_ERR(binary_runtime_measurements);
-		goto out;
-	}
+	if (ima_algo_array[ima_sha1_idx].tfm) {
+		binary_runtime_measurements =
+		    securityfs_create_symlink("binary_runtime_measurements",
+					      ima_dir,
+					      "binary_runtime_measurements_sha1",
+					      NULL);
+		if (IS_ERR(binary_runtime_measurements)) {
+			ret = PTR_ERR(binary_runtime_measurements);
+			goto out;
+		}
 
-	ascii_runtime_measurements =
-	    securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
-				      "ascii_runtime_measurements_sha1", NULL);
-	if (IS_ERR(ascii_runtime_measurements)) {
-		ret = PTR_ERR(ascii_runtime_measurements);
-		goto out;
+		ascii_runtime_measurements =
+		    securityfs_create_symlink("ascii_runtime_measurements",
+					      ima_dir,
+					      "ascii_runtime_measurements_sha1",
+					      NULL);
+		if (IS_ERR(ascii_runtime_measurements)) {
+			ret = PTR_ERR(ascii_runtime_measurements);
+			goto out;
+		}
 	}
 
 	runtime_measurements_count =
-- 
2.49.0

[RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Nicolai Stange]

runtime_measurements_<hash-algo> sysfs files are getting created for
each PCR bank + for SHA-1.

Now that runtime_measurements_<hash-algo> sysfs file creation is being
skipped for unsupported hash algorithms, it will become possible that no
such file would be provided at all once SHA-1 is made optional in a
later patch.

Always create the file for the 'ima_hash' algorithm, even if it's not
associated with any of the PCR banks. As IMA initialization will
continue to fail if the ima_hash algorithm is not available to the
kernel, this guarantees that at least one such file will always be
there.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima_fs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index a8df2fe5f4cb..f030ff7f56da 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void)
 	u16 algo;
 	int i;
 
-	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
-
-	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
-		securityfs_measurement_list_count++;
+	securityfs_measurement_list_count =
+		NR_BANKS(ima_tpm_chip) + ima_extra_slots;
 
 	ascii_securityfs_measurement_lists =
 	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
-- 
2.49.0

[RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Nicolai Stange]

Normally IMA would extend a template hash of each bank's associated
algorithm into a PCR. However, if a bank's hash algorithm is unavailable
to the kernel at IMA init time, it would fallback to extending padded SHA1
hashes instead.

That is, if e.g. SHA-256 was missing at IMA init, it would extend padded
SHA1 template hashes into a PCR's SHA-256 bank.

The ima_measurement command (marked as experimental) from ima-evm-utils
would accordingly try both variants when attempting to verify a measurement
list against PCRs. keylime OTOH doesn't seem to -- it expects the template
hash type to match the PCR bank algorithm. I would argue that for the
latter case, the fallback scheme could potentially cause hard to debug
verification failures.

There's another problem with the fallback scheme: right now, SHA-1
availability is a hard requirement for IMA, and it would be good for a
number of reasons to get rid of that. However, if SHA-1 is not available to
the kernel, it can hardly provide padded SHA-1 template hashes for PCR
banks with unsupported algos.

There are several more or less reasonable alternatives possible, among
them are:
a.) Instead of padded SHA-1, use padded/truncated ima_hash template
    hashes.
b.) Don't extend unsupported banks at all.
c.) Record every event as a violation, i.e. extend unsupported banks
    with 0xffs.
d.) Invalidate unsupported banks at least once by extending with a unique
    constant (e.g. with 0xfes).

a.) would make verification from tools like ima_measurement nearly
    impossible, as it would have to guess or somehow determine ima_hash.
b.) is a security risk, because the bank would validate an empty
    measurement list.
c.) isn't ideal security-wise either, because an unsupported bank would
    then validate an all-violations measurement log.
d.) is the only remaining viable option: extending unsupported PCR banks
    at least once with a unique constant of 0xfe ... fe not used for
    anything else would make those not to validate anything from that
    point on.

For this last alternative d.), there are some variations possible, which
differ in the number of times the magic 0xfe ... fe gets extended into
unsupported banks for invalidation purposes. Among the practical ones
are:
- invalidate each unsupported bank over and over again for each new
  measurement log entry or
- invalidate each unsupported bank exactly once.

The second option has the advantage over the first that it would enable
tools like ima-evm-utils' ima_measurement to recognize unsupported banks
in O(1) time, just by comparing the PCR bank value against the constant
HASH(00 .. 00 | fe .. fe). Note that if OTOH a bank got invalidated over
and over again for each single log entry, and assuming that it's desired
to report unsupported banks as such instead of just failing their
validation, ima_measurement would have to try to match yet another PCR
extension candidate sequence for the 0xfe .. fe over the complete
measurement list, as it's currently being done for the padded SHA1s
template hashes already.

As appealing as the scheme to invalidate each unsupported bank exactly once
might seem at first glance though, there's also the clear drawback of an
additional tracking burden with a significant complexity on the kernel
side: because IMA can't know ahead of time which out of all possible PCRs
would ever get referenced from some policy rules, it cannot simply run the
invalidation of unsupported banks upfront at __init, but would have to do
it lazily upon a given PCR's first extension. The need for carrying the
required state across kexecs, with the possibility of different kernels in
the kexec chain potentially supporting different sets of hash algorithms,
doesn't exactly make things easier either.

So, to move towards the original goal of disentangling IMA from its hard
dependency on SHA-1, go with the more straightforward route for now to
invalidate unsupported PCR banks over and over again for each new
measurement list entry recorded. The more sophisticated
"invalidate-exactly-once" scheme will be left to future patches, if to be
implemented at all.

As this potentially breaks existing userspace, i.e. the current
implementation of ima_measurement, put it behind a Kconfig option,
"IMA_COMPAT_FALLBACK_TPM_EXTEND". If set to "y", the original behavior of
extending with padded SHA-1 is retained. Otherwise the new scheme to
invalidate unsupported PCR banks by extending with constant 0xfe ... fe
will be effective. As ima_measurement is marked as experimental and I find
it unlikely that other existing tools depend on the padded SHA-1 fallback
scheme, make the IMA_COMPAT_FALLBACK_TPM_EXTEND Kconfig option default
to "n".

For IMA_COMPAT_FALLBACK_TPM_EXTEND=n,
- to cover PCR extensions for "regular" measurement list entries, make
  ima_calc_field_array_hash() to fill the digests corresponding to banks
  with unsupported hash algorithms with 0xfes,
- to cover PCR extensions for violation entries, make ima_init_digest() to
  likewise provision the digests[] elements corresponding to unsupported
  banks with 0xfes.

[1] https://github.com/linux-integrity/ima-evm-utils

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/Kconfig      | 14 ++++++++++++++
 security/integrity/ima/ima_crypto.c | 19 ++++++++++++++++++-
 security/integrity/ima/ima_queue.c  | 12 ++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 475c32615006..c8f12a4a4edf 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -122,6 +122,20 @@ config IMA_DEFAULT_HASH
 	default "wp512" if IMA_DEFAULT_HASH_WP512
 	default "sm3" if IMA_DEFAULT_HASH_SM3
 
+config IMA_COMPAT_FALLBACK_TPM_EXTEND
+	bool "Enable compatibility TPM PCR extend for unsupported banks"
+	default n
+	help
+	  In case a TPM PCR hash algorithm is not supported by the kernel,
+	  retain the old behaviour to extend the bank with padded SHA1 template
+	  digests.
+
+	  If Y, IMA will be unavailable when SHA1 is missing from the kernel.
+	  If N, existing tools may fail to verify IMA measurement lists against
+	  TPM PCR banks corresponding to hashes not supported by the kernel.
+
+	  If unsure, say N.
+
 config IMA_WRITE_POLICY
 	bool "Enable multiple writes to the IMA policy"
 	default n
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 6f5696d999d0..a43080fb8edc 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -625,26 +625,43 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
 	u16 alg_id;
 	int rc, i;
 
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
 	if (rc)
 		return rc;
 
 	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
+#endif
 
 	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 		if (i == ima_sha1_idx)
 			continue;
+#endif
 
 		if (i < NR_BANKS(ima_tpm_chip)) {
 			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
 			entry->digests[i].alg_id = alg_id;
 		}
 
-		/* for unmapped TPM algorithms digest is still a padded SHA1 */
+		/*
+		 * For unmapped TPM algorithms, the digest is still a
+		 * padded SHA1 if backwards-compatibility fallback PCR
+		 * extension is enabled. Otherwise fill with
+		 * 0xfes. This is the value to invalidate unsupported
+		 * PCR banks with. Also, a non-all-zeroes value serves
+		 * as an indicator to kexec measurement restoration
+		 * that the entry is not a violation and all its
+		 * template digests need to get recomputed.
+		 */
 		if (!ima_algo_array[i].tfm) {
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 			memcpy(entry->digests[i].digest,
 			       entry->digests[ima_sha1_idx].digest,
 			       TPM_DIGEST_SIZE);
+#else
+			memset(entry->digests[i].digest, 0xfe, TPM_DIGEST_SIZE);
+#endif
 			continue;
 		}
 
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 83d53824aa98..0cc1189446a8 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -274,11 +274,23 @@ int __init ima_init_digests(void)
 		digest_size = ima_tpm_chip->allocated_banks[i].digest_size;
 		crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
 
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 		/* for unmapped TPM algorithms digest is still a padded SHA1 */
 		if (crypto_id == HASH_ALGO__LAST)
 			digest_size = SHA1_DIGEST_SIZE;
 
 		memset(digests[i].digest, 0xff, digest_size);
+#else
+		if (ima_algo_array[i].tfm) {
+			memset(digests[i].digest, 0xff, digest_size);
+		} else {
+			/*
+			 * Unsupported banks are invalidated with 0xfe ... fe
+			 * to disambiguate from violations.
+			 */
+			memset(digests[i].digest, 0xfe, digest_size);
+		}
+#endif
 	}
 
 	return 0;
-- 
2.49.0

[RFC PATCH v2 04/13] ima: make SHA1 non-mandatory

[Nicolai Stange]

For CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND=n, SHA-1 is not a hard
requirement anymore. Make ima_init_crypto() continue on SHA-1
instantiation errors.

Note that the configured ima_hash must still be available. If that
happened to be set to SHA-1 and SHA-1 was missing, then IMA would
still fail to initialize.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima_crypto.c | 59 +++++++++++++----------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index a43080fb8edc..4ac4138d98de 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -145,53 +145,47 @@ int __init ima_init_crypto(void)
 		goto out;
 	}
 
+	ima_algo_array[ima_hash_algo_idx].tfm = ima_shash_tfm;
+	ima_algo_array[ima_hash_algo_idx].algo = ima_hash_algo;
+
+	if (ima_hash_algo != HASH_ALGO_SHA1) {
+		ima_algo_array[ima_sha1_idx].tfm =
+			ima_alloc_tfm(HASH_ALGO_SHA1);
+		if (IS_ERR(ima_algo_array[ima_sha1_idx].tfm)) {
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
+			/*
+			 * For backwards compatible fallback PCR
+			 * extension, SHA1 is the fallback for missing
+			 * algos.
+			 */
+			rc = PTR_ERR(ima_algo_array[ima_sha1_idx].tfm);
+			goto out_array;
+#endif
+			ima_algo_array[ima_sha1_idx].tfm = NULL;
+		}
+		ima_algo_array[ima_sha1_idx].algo = HASH_ALGO_SHA1;
+	}
+
 	for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
 		algo = ima_tpm_chip->allocated_banks[i].crypto_id;
 		ima_algo_array[i].algo = algo;
 
+		/* Initialized separately above. */
+		if (i == ima_hash_algo_idx || i == ima_sha1_idx)
+			continue;
+
 		/* unknown TPM algorithm */
 		if (algo == HASH_ALGO__LAST)
 			continue;
 
-		if (algo == ima_hash_algo) {
-			ima_algo_array[i].tfm = ima_shash_tfm;
-			continue;
-		}
-
 		ima_algo_array[i].tfm = ima_alloc_tfm(algo);
 		if (IS_ERR(ima_algo_array[i].tfm)) {
-			if (algo == HASH_ALGO_SHA1) {
-				rc = PTR_ERR(ima_algo_array[i].tfm);
-				ima_algo_array[i].tfm = NULL;
-				goto out_array;
-			}
-
 			ima_algo_array[i].tfm = NULL;
 		}
 	}
 
-	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip)) {
-		if (ima_hash_algo == HASH_ALGO_SHA1) {
-			ima_algo_array[ima_sha1_idx].tfm = ima_shash_tfm;
-		} else {
-			ima_algo_array[ima_sha1_idx].tfm =
-						ima_alloc_tfm(HASH_ALGO_SHA1);
-			if (IS_ERR(ima_algo_array[ima_sha1_idx].tfm)) {
-				rc = PTR_ERR(ima_algo_array[ima_sha1_idx].tfm);
-				goto out_array;
-			}
-		}
-
-		ima_algo_array[ima_sha1_idx].algo = HASH_ALGO_SHA1;
-	}
-
-	if (ima_hash_algo_idx >= NR_BANKS(ima_tpm_chip) &&
-	    ima_hash_algo_idx != ima_sha1_idx) {
-		ima_algo_array[ima_hash_algo_idx].tfm = ima_shash_tfm;
-		ima_algo_array[ima_hash_algo_idx].algo = ima_hash_algo;
-	}
-
 	return 0;
+#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 out_array:
 	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
 		if (!ima_algo_array[i].tfm ||
@@ -201,6 +195,7 @@ int __init ima_init_crypto(void)
 		crypto_free_shash(ima_algo_array[i].tfm);
 	}
 	kfree(ima_algo_array);
+#endif
 out:
 	crypto_free_shash(ima_shash_tfm);
 	return rc;
-- 
2.49.0

[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256 from Kconfig

[Nicolai Stange]

Since recently, IMA would not record measurement list entries into PCR
banks for which it doesn't have a corresponding in-kernel hash algorithm
implementation available anymore (for
CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND=n).

With TPM 2.0, the only hash algorithms guaranteed to be implemented on a
TPM are SHA-256/384, c.f. "TCG PC Client Platform TPM Profile
Specification for TPM 2.0", sec. 4.6 "PCR Requirements".

In particular, sha1 is not mandatory, and thus, the CRYPTO_SHA1 dependency
of IMA is not sufficient anymore for ensuring that IMA would find at least
one usable PCR bank.

So, in order to make sure that IMA has access to at least one usable bank
on platforms featuring a TPM 2.0 device, make it depend on CRYPTO_SHA256.

Keep the dependency on CRYPTO_SHA1 for the TPM 1 case.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index c8f12a4a4edf..8a7e74dc1477 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -7,6 +7,7 @@ config IMA
 	select CRYPTO
 	select CRYPTO_HMAC
 	select CRYPTO_SHA1
+	select CRYPTO_SHA256
 	select CRYPTO_HASH_INFO
 	select SECURITY_PATH
 	select TCG_TPM if HAS_IOMEM
-- 
2.49.0

[RFC PATCH v2 06/13] ima: move INVALID_PCR() to ima.h

[Nicolai Stange]

Make the INVALID_PCR() #define available to other compilation units
by moving it from ima_policy.c to ima.h and renaming it to
IMA_INVALID_PCR() in the course.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h        | 4 ++++
 security/integrity/ima/ima_policy.c | 5 +----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a4f284bd846c..1158a7b8bf6b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -198,6 +198,10 @@ struct ima_iint_cache {
 	struct ima_digest_data *ima_hash;
 };
 
+#define IMA_INVALID_PCR(a) (((a) < 0) || \
+	(a) >= (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
+
+
 extern struct lsm_blob_sizes ima_blob_sizes;
 
 static inline struct ima_iint_cache *
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 128fab897930..d9e4210ea814 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -48,9 +48,6 @@
 #define HASH		0x0100
 #define DONT_HASH	0x0200
 
-#define INVALID_PCR(a) (((a) < 0) || \
-	(a) >= (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
-
 int ima_policy_flag;
 static int temp_ima_appraise;
 static int build_ima_appraise __ro_after_init;
@@ -1855,7 +1852,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			ima_log_string(ab, "pcr", args[0].from);
 
 			result = kstrtoint(args[0].from, 10, &entry->pcr);
-			if (result || INVALID_PCR(entry->pcr))
+			if (result || IMA_INVALID_PCR(entry->pcr))
 				result = -EINVAL;
 			else
 				entry->flags |= IMA_PCR;
-- 
2.49.0

[RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Nicolai Stange]

The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
the corresponding digest from the provided digests[] argument.

An upcoming code change to IMA will introduce the need to skip over those
banks it does not have a hash algorithm implementation available for.

Introduce tpm_pcr_extend_sel() to support this.

tpm_pcr_extend_sel() also expects a digests[] array, always being the
number of allocated PCR banks in size, just as it's the case for the
existing tpm_pcr_extend(). In addition to that however, it takes a
'banks_skip_mask', and will skip the extension of any bank having its
corresponding bit set there.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
 drivers/char/tpm/tpm.h           |  3 ++-
 drivers/char/tpm/tpm2-cmd.c      | 29 +++++++++++++++++++++++++++--
 include/linux/tpm.h              |  3 +++
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b1daa0d7b341..88b4496de1df 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  */
 int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 		   struct tpm_digest *digests)
+{
+	return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
+}
+EXPORT_SYMBOL_GPL(tpm_pcr_extend);
+
+/**
+ * tpm_pcr_extend_sel - extend a PCR value into selected banks.
+ * @chip:	a &struct tpm_chip instance, %NULL for the default chip
+ * @pcr_idx:	the PCR to be retrieved
+ * @digests:	array of tpm_digest structures used to extend PCRs
+ * @banks_skip_mask:	pcr banks to skip
+ *
+ * Note: callers must pass a digest for every allocated PCR bank, in the same
+ * order of the banks in chip->allocated_banks.
+ *
+ * Return: same as with tpm_transmit_cmd()
+ */
+int tpm_pcr_extend_sel(struct tpm_chip *chip, u32 pcr_idx,
+		       struct tpm_digest *digests,
+		       unsigned long banks_skip_mask)
 {
 	int rc;
 	int i;
@@ -330,7 +350,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	}
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		rc = tpm2_pcr_extend(chip, pcr_idx, digests);
+		rc = tpm2_pcr_extend(chip, pcr_idx, digests, banks_skip_mask);
+		goto out;
+	}
+
+	/* There's only one SHA1 bank with TPM 1. */
+	if (banks_skip_mask & 1) {
+		rc = 0;
 		goto out;
 	}
 
@@ -341,7 +367,6 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	tpm_put_ops(chip);
 	return rc;
 }
-EXPORT_SYMBOL_GPL(tpm_pcr_extend);
 
 int tpm_auto_startup(struct tpm_chip *chip)
 {
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7bb87fa5f7a1..f4ed49cb4101 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -291,7 +291,8 @@ 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_extend(struct tpm_chip *chip, u32 pcr_idx,
-		    struct tpm_digest *digests);
+		    struct tpm_digest *digests,
+		    unsigned long banks_skip_mask);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 			u32 *value, const char *desc);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index dfdcbd009720..23ded8ea47dc 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR.
  * @digests:	list of pcr banks and corresponding digest values to extend.
+ * @banks_skip_mask:	pcr banks to skip
  *
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
-		    struct tpm_digest *digests)
+		    struct tpm_digest *digests,
+		    unsigned long banks_skip_mask)
 {
 	struct tpm_buf buf;
+	unsigned long skip_mask;
+	u32 banks_count;
 	int rc;
 	int i;
 
+	banks_count = 0;
+	skip_mask = banks_skip_mask;
+	for (i = 0; i < chip->nr_allocated_banks; i++) {
+		const bool skip_bank = skip_mask & 1;
+
+		skip_mask >>= 1;
+		if (skip_bank)
+			continue;
+		banks_count++;
+	}
+
+	if (banks_count == 0)
+		return 0;
+
 	if (!disable_pcr_integrity) {
 		rc = tpm2_start_auth_session(chip);
 		if (rc)
@@ -257,9 +275,16 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
 	}
 
-	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+	tpm_buf_append_u32(&buf, banks_count);
 
+	skip_mask = banks_skip_mask;
 	for (i = 0; i < chip->nr_allocated_banks; i++) {
+		const bool skip_bank = skip_mask & 1;
+
+		skip_mask >>= 1;
+		if (skip_bank)
+			continue;
+
 		tpm_buf_append_u16(&buf, digests[i].alg_id);
 		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
 			       chip->allocated_banks[i].digest_size);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 20a40ade8030..7587eecc82fd 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -447,6 +447,9 @@ extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 			struct tpm_digest *digest);
 extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 			  struct tpm_digest *digests);
+extern int tpm_pcr_extend_sel(struct tpm_chip *chip, u32 pcr_idx,
+			      struct tpm_digest *digests,
+			      unsigned long banks_skip_mask);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
 extern struct tpm_chip *tpm_default_chip(void);
 void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
-- 
2.49.0

[RFC PATCH v2 08/13] ima: track the set of PCRs ever extended

[Nicolai Stange]

Right now, PCR banks with unsupported hash algorithms are getting
invalidated over and over again for each new measurement list entry
recorded.

A subsequent patch will make IMA to invalidate PCR banks associated with
unsupported hash algorithms only once at a PCR's first use. To prepare for
that, make it track the set of PCRs ever extended.

Maintain the set of touched PCRs in an unsigned long bitmask,
'ima_extended_pcrs_mask'.

Amend the IMA_INVALID_PCR() #define to check that a given PCR can get
represented in that bitmask. Note that this is only for improving code
maintainablity, it does not actually constain the set of allowed PCR
indices any further.

Make ima_pcr_extend() to maintain the ima_extended_pcrs_mask, i.e. to set
the currently extented PCR's corresponding bit.

Note that at this point there's no provision to restore the
ima_extended_pcrs_mask value after kexecs yet, that will be the subject of
later patches.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h       |  8 ++++++--
 security/integrity/ima/ima_queue.c | 17 +++++++++++++----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 1158a7b8bf6b..f99b1f81b35c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -20,6 +20,7 @@
 #include <linux/hash.h>
 #include <linux/tpm.h>
 #include <linux/audit.h>
+#include <linux/minmax.h>
 #include <crypto/hash_info.h>
 
 #include "../integrity.h"
@@ -62,6 +63,8 @@ extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
 extern struct ima_algo_desc *ima_algo_array __ro_after_init;
 
+extern unsigned long ima_extended_pcrs_mask;
+
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
 extern const char boot_aggregate_name[];
@@ -198,8 +201,9 @@ struct ima_iint_cache {
 	struct ima_digest_data *ima_hash;
 };
 
-#define IMA_INVALID_PCR(a) (((a) < 0) || \
-	(a) >= (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
+#define IMA_INVALID_PCR(a) (((a) < 0) ||				    \
+	(a) >= (8 * min(sizeof_field(struct ima_iint_cache, measured_pcrs), \
+			sizeof(ima_extended_pcrs_mask))))
 
 
 extern struct lsm_blob_sizes ima_blob_sizes;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 0cc1189446a8..6e8a7514d9f6 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -51,6 +51,11 @@ static DEFINE_MUTEX(ima_extend_list_mutex);
  */
 static bool ima_measurements_suspended;
 
+/*
+ * Set of PCRs ever extended by IMA.
+ */
+unsigned long ima_extended_pcrs_mask;
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -144,15 +149,19 @@ unsigned long ima_get_binary_runtime_size(void)
 
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
-	int result = 0;
+	int result;
 
 	if (!ima_tpm_chip)
-		return result;
+		return 0;
 
 	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
-	if (result != 0)
+	if (result != 0) {
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
-	return result;
+		return result;
+	}
+
+	ima_extended_pcrs_mask |= BIT(pcr);
+	return 0;
 }
 
 /*
-- 
2.49.0

[RFC PATCH v2 09/13] ima: invalidate unsupported PCR banks only once

[Nicolai Stange]

As it currently stands, IMA would invalidate a PCR bank corresponding to
an unsupported hash algorithm over and over again by extending it with
0xfe ... fe for each measurement list entry recorded (for
CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND=n).

This however, makes the problem of deciding from the PCR bank value
whether or not the bank had been invalidated or not linear in the
measurement list length: one would have to reproduce the potential
invalidation extension sequence incrementally for each event recorded
and compare whether the PCR bank value would match at any given point.

From a soundness POV, the repeated invalidations are not needed: a single
one would suffice to ensure no verifier would (wrongly) verify any
measurement list against the invalidated PCR bank value.

With only a single invalidation, an invalidated PCR bank value can get
recognized easily in O(1) by comparing against
HASH(0x00 ... 00 | fe ... fe), i.e. the value a PCR bank would have if
invalidated once in its initial state.

This would potentially help userspace tools such as ima-evm-utils
ima_measurement ([1]) which might want to filter unmaintained PCR banks
and also, it will enable the kernel to log some meaningful meassages when
the set of supported hash algorithm differs between kernel instances in a
kexec chain.

Start out by invalidating unsupported PCR banks exactly once from each
kernel in a kexec chain. Skipping re-invalidations from subsequent kernels
in a kexec chain will be the subject of a future patch.

Make IMA's crypto __init code to fill in a bitmask of banks with
unsupported hash algorithms, ima_unsupported_pcr_banks_mask.
ima_unsupported_pcr_banks_mask has been chosen to be of type unsigned
long, with the expectation that it will be sufficient to represent all
banks allocated on a TPM in practice -- note that 32 > the number of hash
algorithm identifiers defined by the TCG. If not, the code would
implictly fall back to re-invalidating "excess" banks over and over again,
so it is always sound.

Make ima_pcr_extend() to skip the extension of a PCR's unsupported banks
in case the given PCR had not been extend before, as already tracked in
the ima_extended_pcrs_mask introduced by a previous patch. That is,
invalidate unsupported banks only once at any given PCR's first extension.

Note that ima_extended_pcrs_mask is not retained across kernels in a
kexec chain, so each booted kernel would re-invalidate the unsupported
banks again. As said above, taking care of this as well will be handled
in a separate patch.

[1] https://github.com/linux-integrity/ima-evm-utils.git

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h        |  1 +
 security/integrity/ima/ima_crypto.c | 21 +++++++++++++++++----
 security/integrity/ima/ima_queue.c  | 18 +++++++++++++++++-
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f99b1f81b35c..7ad4a1eefd94 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -62,6 +62,7 @@ extern int ima_sha1_idx __ro_after_init;
 extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
 extern struct ima_algo_desc *ima_algo_array __ro_after_init;
+extern unsigned long ima_unsupported_pcr_banks_mask __ro_after_init;
 
 extern unsigned long ima_extended_pcrs_mask;
 
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 4ac4138d98de..c78bf4872b6a 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -67,6 +67,8 @@ int ima_extra_slots __ro_after_init;
 
 struct ima_algo_desc *ima_algo_array __ro_after_init;
 
+unsigned long ima_unsupported_pcr_banks_mask __ro_after_init;
+
 static int __init ima_init_ima_crypto(void)
 {
 	long rc;
@@ -184,6 +186,16 @@ int __init ima_init_crypto(void)
 		}
 	}
 
+	for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
+		if (i >= BITS_PER_LONG) {
+			pr_warn("Too many TPM PCR banks, invalidation tracking capped");
+			break;
+		}
+
+		if (!ima_algo_array[i].tfm)
+			ima_unsupported_pcr_banks_mask |= BIT(i);
+	}
+
 	return 0;
 #if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
 out_array:
@@ -644,10 +656,11 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
 		 * padded SHA1 if backwards-compatibility fallback PCR
 		 * extension is enabled. Otherwise fill with
 		 * 0xfes. This is the value to invalidate unsupported
-		 * PCR banks with. Also, a non-all-zeroes value serves
-		 * as an indicator to kexec measurement restoration
-		 * that the entry is not a violation and all its
-		 * template digests need to get recomputed.
+		 * PCR banks with once at first use. Also, a
+		 * non-all-zeroes value serves as an indicator to
+		 * kexec measurement restoration that the entry is not
+		 * a violation and all its template digests need to
+		 * get recomputed.
 		 */
 		if (!ima_algo_array[i].tfm) {
 #if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 6e8a7514d9f6..83d5c7034919 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -150,11 +150,27 @@ unsigned long ima_get_binary_runtime_size(void)
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
 	int result;
+	unsigned long pcr_banks_skip_mask;
 
 	if (!ima_tpm_chip)
 		return 0;
 
-	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
+#if !IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
+	pcr_banks_skip_mask = ima_unsupported_pcr_banks_mask;
+	if (!(ima_extended_pcrs_mask & BIT(pcr))) {
+		/*
+		 * Invalidate unsupported banks once upon a PCR's
+		 * first usage. Note that the digests_arg[] entries for
+		 * unsupported algorithms have been filled with 0xfes.
+		 */
+		pcr_banks_skip_mask = 0;
+	}
+#else
+	pcr_banks_skip_mask = 0;
+#endif
+
+	result = tpm_pcr_extend_sel(ima_tpm_chip, pcr, digests_arg,
+				    pcr_banks_skip_mask);
 	if (result != 0) {
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 		return result;
-- 
2.49.0

[RFC PATCH v2 10/13] tpm: authenticate tpm2_pcr_read()

[Nicolai Stange]

PCR reads aren't currently authenticated even with CONFIG_TCG_TPM2_HMAC=y
yet.

It is probably desirable though, as e.g. IMA does some PCR reads to form
the cumulative boot digest subsequently extended into PCR 10 (an operation
which *is* authenticated).

Furthermore, a subsequent patch will make IMA to skip certain PCR bank
re-invalidations (which are implemented with extensions) upon kexec based
on the value read back at boot. In order to not weaken the overall
security posture in this case, it will be required to establish the same
level of trust into PCR reads as there is already for the extensions.

Make tpm2_pcr_read() to protect the command with a HMAC auth session,
using the already existing infrastructure.

As the TPM2_PCR_Read command doesn't have any authorizations defined, and
neither of TPM2_SA_ENCRYPT/TPM2_SA_DECRYPT is needed, use TPM2_SA_AUDIT,
even though no auditing functionality is actually being required. Since
the TPM will set TPM2_SA_AUDIT_EXCLUSIVE in its response with this
single-use session, set it upfront so that tpm_buf_check_hmac_response()
would expect it for the HMAC verification.

Now that tpm2_pcr_read() depends on the driver's session infrastructure,
note that the first call to tpm2_pcr_read() at init time gets issued from
  tpm_chip_bootstrap() -> tpm_get_pcr_allocation()
  -> tpm2_get_pcr_allocation() -> tpm2_init_bank_info()
  -> tpm2_pcr_read()
after
  tpm_chip_bootstrap() -> tpm_auto_startup() -> tpm2_auto_startup()
  -> tpm2_sessions_init(),
so there won't be any issues with that.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 drivers/char/tpm/tpm2-cmd.c | 46 +++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 23ded8ea47dc..e16772bbc5c8 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -168,6 +168,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 	int i;
 	int rc;
 	struct tpm_buf buf;
+	struct tpm_header *head;
+	int offset_p;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
 	u16 digest_size;
@@ -187,9 +189,30 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		expected_digest_size = chip->allocated_banks[i].digest_size;
 	}
 
-	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
-	if (rc)
-		return rc;
+	if (IS_ENABLED(CONFIG_TCG_TPM2_HMAC) && !disable_pcr_integrity) {
+		rc = tpm2_start_auth_session(chip);
+		if (rc)
+			return rc;
+
+		rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_READ);
+		if (rc) {
+			tpm2_end_auth_session(chip);
+			return rc;
+		}
+
+		/*
+		 * Exclusivity is not needed, but set in the response.
+		 * Set it here too, so that the HMAC verification
+		 * won't fail.
+		 */
+		tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_AUDIT
+					    | TPM2_SA_AUDIT_EXCLUSIVE,
+					    NULL, 0);
+	} else {
+		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);
 
@@ -199,11 +222,24 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
+	if (!disable_pcr_integrity)
+		tpm_buf_fill_hmac_session(chip, &buf);
 	rc = tpm_transmit_cmd(chip, &buf, 0, "attempting to read a pcr value");
 	if (rc)
 		goto out;
+	if (!disable_pcr_integrity) {
+		rc = tpm_buf_check_hmac_response(chip, &buf, rc);
+		if (rc)
+			goto out;
+	}
+
+	head = (struct tpm_header *)buf.data;
+	offset_p = TPM_HEADER_SIZE;
+	/* Skip the parameter size field: */
+	if (be16_to_cpu(head->tag) == TPM2_ST_SESSIONS)
+		offset_p += 4;
+	out = (struct tpm2_pcr_read_out *)&buf.data[offset_p];
 
-	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
 	digest_size = be16_to_cpu(out->digest_size);
 	if (digest_size > sizeof(digest->digest) ||
 	    (!digest_size_ptr && digest_size != expected_digest_size)) {
@@ -216,6 +252,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 
 	memcpy(digest->digest, out->digest, digest_size);
 out:
+	if (!disable_pcr_integrity)
+		tpm2_end_auth_session(chip);
 	tpm_buf_destroy(&buf);
 	return rc;
 }
-- 
2.49.0

[RFC PATCH v2 11/13] ima: introduce ima_pcr_invalidated_banks() helper

[Nicolai Stange]

At the current stage, IMA would invalidate PCR banks corresponding to
unsupported hash algorithms exactly once by extending with the special
0xfe ... fe from each kernel in a kexec chain.

In order to work towards the goal of doing that only once for the
overall chain, subsequent kernels must be able to recognize already
invalidated PCR banks.

PCR banks invalidated when in their initial reset state would have a value
of HASH(0x00 ... 00 | fe ... fe).

Introduce the ima_pcr_invalidated_banks() implementing this comparison for
a couple of selected hash algorithms, namely the set the current TPM
driver code knows about.

Note that false positives would be fatal as far as soundness is concerned,
as a future patch will make IMA to skip invalidations for banks reported
to have been invalidated already. False negatives however will only cause
superfluous re-invalidations, i.e. a PCR bank would not be recognizable as
unsupported anymore, but any attempt to verify a measurement list against
it would still fail. Thus, ima_pcr_invalidated_banks() doesn't necessarily
need to support every hash algorithm possible and in particular, failure
to keep it in sync with the TPM driver code, should the latter learn about
some more hash algorithms in the future, would not be an issue.

Let ima_pcr_invalidated_banks() read back all of a given PCR's bank
digests from the TPM. Attempt to compare each against the well-known
value of HASH(0x00 ... 00 | fe ... fe) and, in case of a match, set the
corresponding bit in a bitmask returned eventually back to the caller.

The type chosen for the returned bitmask is unsigned long. If the number
of allocated banks exceeds its width, stop early after BITS_PER_LONG banks
have been examined -- as mention earlier false negatives aren't fatal.

In order to enable ima_pcr_invalidated_banks() to make comparisons against
those well-known HASH(...) values from above, even in the scenario of
interest here where the kernel's crypto API is lacking a usable
implementation for some hash, provide them as pre-computed values in a
lookup table for a number of selected hash algorithms, namely those
recognized by the current TPM driver code.

Lastly a word of caution towards the cherry-pickers among us: you will
likely also want that other patch to the TPM driver code making
tpm2_pcr_read() to authenticate the TPM response -- otherwise an
interposer could potentially trick IMA to skip a needed PCR bank
invalidation from a kexeced kernel even with CONFIG_TCG_TPM2_HMAC=y.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h        |   1 +
 security/integrity/ima/ima_crypto.c | 125 ++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7ad4a1eefd94..67b78f5512f1 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -274,6 +274,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 		       struct ima_iint_cache *iint, const char *op,
 		       const char *cause);
 int ima_init_crypto(void);
+unsigned long ima_pcr_invalidated_banks(u32 pcr);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
 int template_desc_init_fields(const char *template_fmt,
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index c78bf4872b6a..c1d9cd85a66d 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -900,3 +900,128 @@ int ima_calc_boot_aggregate(struct ima_digest_data *hash)
 
 	return rc;
 }
+
+/*
+ * Expected values for unsupported PCR banks after invalidating once
+ * with 0xfe ... fe, i.e. HASH(0x00 ... 00 fe ... fe).
+ * The list might not be exhaustive as far as the set of recognized
+ * algorithms is concerned.
+ */
+static const struct {
+	u16 tpm_alg_id;
+	u8 digest_length;
+	const u8 *digest;
+} unsupported_pcr_bank_values[] = {
+	{
+		TPM_ALG_SHA1, 20,
+		(const u8[]) {
+			0x74, 0x43, 0x5f, 0x39, 0xb5, 0x05, 0x21, 0x26,
+			0x9d, 0xaa, 0xfd, 0x3e, 0x11, 0x1b, 0xf1, 0xd9,
+			0x14, 0x1d, 0x9a, 0x5f,
+		},
+	},
+	{
+		TPM_ALG_SHA256, 32,
+		(const u8[]) {
+			0x7a, 0x42, 0xe1, 0xf2, 0x6c, 0x07, 0x82, 0x7f,
+			0xaa, 0x54, 0x87, 0x47, 0x62, 0xfd, 0x7f, 0xe7,
+			0xa1, 0xdf, 0xbb, 0x8f, 0xfa, 0x51, 0xbf, 0x53,
+			0x22, 0xa7, 0x71, 0xd2, 0xc8, 0x80, 0xc5, 0x86,
+		},
+	},
+	{
+		TPM_ALG_SHA384, 48,
+		(const u8[]) {
+			0x68, 0xaa, 0xdf, 0xd3, 0x3e, 0x54, 0x15, 0x40,
+			0x73, 0xc8, 0x6a, 0x95, 0x8d, 0x5d, 0x7b, 0xb2,
+			0x68, 0xf3, 0x0c, 0x14, 0x9e, 0x19, 0x6d, 0x08,
+			0x24, 0x7d, 0x51, 0x26, 0x05, 0xe5, 0x1c, 0x40,
+			0xdd, 0xc8, 0x44, 0x4e, 0x93, 0x8a, 0x37, 0x05,
+			0xfc, 0xd6, 0xa2, 0x80, 0xe3, 0x27, 0x0d, 0x71,
+		},
+	},
+	{
+		TPM_ALG_SHA512, 64,
+		(const u8[]) {
+			0x58, 0x8c, 0x38, 0x64, 0x06, 0xdb, 0x9b, 0xcc,
+			0x26, 0xa4, 0x13, 0x9c, 0x8a, 0xff, 0x6a, 0x10,
+			0xf4, 0xe6, 0x5a, 0x92, 0xbd, 0xed, 0x9d, 0x62,
+			0xbe, 0x92, 0x1b, 0x40, 0xf6, 0x7d, 0x9b, 0xc3,
+			0x0d, 0x07, 0xc8, 0xfb, 0x1a, 0x8d, 0x56, 0xfa,
+			0xa4, 0xf2, 0x05, 0xb6, 0x81, 0x29, 0x14, 0x5f,
+			0xf6, 0x71, 0x32, 0xbb, 0x0d, 0x31, 0xca, 0xf3,
+			0x5e, 0x8e, 0x95, 0xd9, 0xd8, 0x55, 0x28, 0x95,
+		},
+	},
+	{
+		TPM_ALG_SM3_256, 32,
+		(const u8[]) {
+			0x05, 0xff, 0xaf, 0x59, 0x7e, 0x50, 0x39, 0x5b,
+			0xaf, 0x69, 0xc0, 0xdc, 0x19, 0xb0, 0xe0, 0xfe,
+			0x3f, 0x6b, 0x6f, 0x03, 0xcd, 0x04, 0xf6, 0x80,
+			0x6c, 0x59, 0xdc, 0xd2, 0x06, 0xbf, 0x38, 0x78
+		},
+	},
+};
+
+/*
+ * Return true if the supplied PCR digest can get confirmed to match
+ * the expected value a bank with unsupported associated hash algorithm
+ * would have after invalidating it exactly once.
+ * Otherwise, return false.
+ *
+ * False negatives are tolerable from a soundness POV, but would
+ * potentially cause additional re-invalidations e.g. after kexec.
+ */
+static bool is_pcr_bank_invalidated(const struct tpm_digest * const digest)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(unsupported_pcr_bank_values); ++i) {
+		if (unsupported_pcr_bank_values[i].tpm_alg_id == digest->alg_id)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(unsupported_pcr_bank_values))
+		return false;
+
+	return memcmp(unsupported_pcr_bank_values[i].digest, digest->digest,
+		      unsupported_pcr_bank_values[i].digest_length) == 0;
+}
+
+/*
+ * Read all of a PCR's banks and check which of those have a value
+ * matching the expected digest after invalidating once for
+ * unsupported algorithms.
+ *
+ * A bitmask of banks found to have been invalidated is getting
+ * returned. The set is not guaranteed to be complete.
+ */
+unsigned long ima_pcr_invalidated_banks(u32 pcr)
+{
+	int i, r;
+	struct tpm_digest d;
+	unsigned long invalidated_banks_mask = 0;
+
+	for (i = 0; i < NR_BANKS(ima_tpm_chip); i++) {
+		if (i >= BITS_PER_LONG)
+			break;
+
+		d.alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
+		r = tpm_pcr_read(ima_tpm_chip, pcr, &d);
+		if (r) {
+			/*
+			 * Failure to read is non-fatal, emit a
+			 * warning only and move on to the next bank.
+			 */
+			pr_warn("TPM PCR read failed %d, pcr=%d, bank=0x%02x\n",
+				r, pcr,
+				ima_tpm_chip->allocated_banks[i].alg_id);
+		}
+
+		if (is_pcr_bank_invalidated(&d))
+			invalidated_banks_mask |= BIT(i);
+	}
+
+	return invalidated_banks_mask;
+}
-- 
2.49.0

[RFC PATCH v2 12/13] ima: make ima_free_tfm()'s linkage extern

[Nicolai Stange]

Upon recognizing previously unmaintained PCR banks at __init after kexec,
a subsequent commit will make IMA to disable the corresponding hashes
for the current boot as well.

For this, access to ima_free_tfm() from outside its compilation unit is
needed. Make its linkage extern.

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h        | 1 +
 security/integrity/ima/ima_crypto.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 67b78f5512f1..9bfe045ac9d5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -274,6 +274,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
 		       struct ima_iint_cache *iint, const char *op,
 		       const char *cause);
 int ima_init_crypto(void);
+void ima_free_tfm(struct crypto_shash *tfm);
 unsigned long ima_pcr_invalidated_banks(u32 pcr);
 void ima_putc(struct seq_file *m, void *data, int datalen);
 void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index c1d9cd85a66d..716bb302e75d 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -213,7 +213,7 @@ int __init ima_init_crypto(void)
 	return rc;
 }
 
-static void ima_free_tfm(struct crypto_shash *tfm)
+void ima_free_tfm(struct crypto_shash *tfm)
 {
 	int i;
 
-- 
2.49.0

[RFC PATCH v2 13/13] ima: don't re-invalidate unsupported PCR banks after kexec

[Nicolai Stange]

Currently, IMA would invalidate a PCR bank corresponding to an unsupported
hash algorithm by extending it with 0xfe ... fe exactly once.

The idea is, that banks corresponding to such algorithms would be readily
recognizable by comparing against HASH(0x00 ... 00 | fe ... fe), i.e. the
value they'd have if extended once when in their initial state after
reset.

However, PCRs don't get reset at kexec, yet each kernel in a kexec chain
would do the (re-)invalidation again. That means that a PCR's bank would
have a value of e.g. HASH(HASH(0x00 ... 00 | fe ... fe) | fe ... fe)
after a re-invalidation from the second kernel in the chain and so on.
In this scenario, it's not anymore immediate from the PCR bank value
alone whether an invalidation has happened or not.

Don't re-invalidate banks found to have a value of
HASH(0x00 ... 00 | fe ... fe) again after kexec.

Note that the assumption implicit to the example above was that all
kernels in a kexec chain wouldn't have support for a given hash algorithm.
However, as it's pointless to start extending some already invalidated
bank somewhere in the middle of a kexec chain, an even more general
approach is possible: don't /extend/ banks found to have a value of
HASH(0x00 ... 00 | fe ... fe) ever again after kexec.

With that, any bank corresponding to a hash algorithm not supported by the
initial kernel in a kexec chain will retain its
HASH(0x00 ... 00 | fe ... fe) value, independent of whether subsequent
kernels do or do not support the algorithm in question (assuming all
kernels in the chain implement this logic, of course).

If some bank's hash algorithm is initially supported by one or more
kernels at the head of the kexec chain, but ceases to do so at a certain
later point, it necessarily must get invalidated then. As the PCR bank
cannot get moved to the magic of HASH(0x00 ... 00 | fe ... fe) by design
of how PCRs work anyway, it doesn't matter how many times it's getting
invalidated, as long as it's being done at least once. For practicality
reasons, the choice has been made to re-invalidate such "discontinued"
banks for every new measurement list entry over and over again.
(Otherwise, if those banks were e.g. to get invalidated exactly once from
 each kernel not supporting their hashes, the information to get tracked
 per PCR would become a tristate: never extended before, extended by a
 previous kernel or extended by the current one already -- the sets of
 banks to invalidate upon measurement list entry addition would
 potentially all be different for each case.)

So in summary:
- don't touch banks not supported by the initial kernel ever again and
  keep them at their magic HASH(0x00 ... 00 | fe ... fe) values,
- (re-)invalidate any additional bank not supported by the current kernel
  over and over again for each new measurement list entry added.

Regarding the implementation:
- Make ima_restore_measurement_list() to determine the set of banks in
  magic HASH(0x00 ... 00 | fe ... fe) state for each PCR referenced from
  any entry in the preexisting measurement list after kexec. This is being
  done by reading back via the ima_pcr_invalidated_banks() helper
  introduced with a previous patch. The result of this step is the set
  of banks found invalidated for all the PCRs referenced, kept in the
  local always_invalidated_pcrs_banks_mask bitmask.

  While walking the measurement list, make
  ima_restore_measurement_list() to also update the already existing
  global ima_extended_pcrs_mask to record every PCR seen.

- After walking through the measurement list,
  always_invalidated_pcrs_banks_mask represents the set of banks whose
  associated hash algorithms had not been supported by the first kernel
  in the kexec chain and thus, not been updated by any subsequent kernel
  between the first and the current one accordingly.

  At that point, ima_unsupported_pcr_banks_mask corresponds to the set of
  banks whose associated hash algorithms aren't supported by the current
  kernel, as per the initialization in ima_init_crypto().

  The set difference between the two, i.e.
    ima_unsupported_pcr_banks_mask & ~always_invalidated_pcrs_banks_mask
  represents the set of "discontinued" banks, i.e. banks that had been
  supported and extended before, but which the current kernel cannot
  maintain any further. As explained above, this set of banks will be
  subject to re-invalidation upon each new measurement list entry
  addition. Store it away in new global ima_discontinued_pcr_banks_mask.

- After ima_unsupported_pcr_banks_mask has been computed, update
  ima_unsupported_pcr_banks_mask to the union of itself (the algorithms
  not supported by the current kernel) with the set of algorithms found to
  not have been supported by the first kernel in the kexec chain, as
  represented by the always_invalidated_pcrs_banks_mask from above.

  At this point, the set banks to not extend any further (for any PCR
  already encountered before) is
    ima_unsupported_pcr_banks_mask & ~ima_discontinued_pcr_banks_mask.

  For clarification: for the first kernel in a kexec chain
  ima_discontinued_pcr_banks_mask is zero and the result of the
  expression above is ima_unsupported_pcr_banks_mask. For a subsequent
  kernel however, this effectively evaluates to the
  always_invalidated_pcrs_banks_mask found above, which should in turn
  likewise match the set of algorithms not supported by the initial
  kernel.

  Either way, make ima_pcr_extend() to skip over banks in the set
  represented by the above expression, instead of over the plain
  ima_unsupported_pcr_banks_mask as before.

- The last missing piece is to handle PCRs never encountered before
  correctly. When subsequently extended for the first time, these must
  get all their banks in ima_unsupported_pcr_banks_mask invalidated,
  hence extended with 0xfe ... fe. IMA's template hash computation
  primitives (more specifically ima_calc_field_array_hash() and
  ima_init_digests()) would substitute 0xfe ... fe for a bank iff the
  associated ima_algo_array[]'s entry's ->tfm is NULL.

  Make ima_restore_measurement_list() to reset any ->tfm potentially
  non-NULL but in ima_unsupported_pcr_banks_mask. That is, make it reset
  ->tfm's for which the current kernel does have an implementation
  available, but the first kernel in the kexec chain did not.

  As a welcomed side-effect this will also suppress the creation of the
  {binary,ascii}_runtime_measurements_<hash> sysfs files for any bank
  never maintained throughout the kexec chain.

- Lastly, while iterating over the banks and resetting ->tfm's as
  appropriate, log some useful information:
  - inform about banks found unsupported throughout the kexec chain, i.e.
    in magic HASH(0x00 ... 00 | fe ... fe) state,
  - print an error if the bank associated with ima_hash is among those
  - and warn about discontinued banks.

  With that, any unsupported bank will leave a trace at least once in a
  kernel log somewhere in the kexec chain (assuming all the kernels
  are implementing this logic, of course).

Signed-off-by: Nicolai Stange <nstange@suse.de>
---
 security/integrity/ima/ima.h          |  1 +
 security/integrity/ima/ima_crypto.c   |  6 ++
 security/integrity/ima/ima_queue.c    |  9 ++-
 security/integrity/ima/ima_template.c | 84 ++++++++++++++++++++++++++-
 4 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 9bfe045ac9d5..2b5351cbf6bd 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -63,6 +63,7 @@ extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
 extern struct ima_algo_desc *ima_algo_array __ro_after_init;
 extern unsigned long ima_unsupported_pcr_banks_mask __ro_after_init;
+extern unsigned long ima_discontinued_pcr_banks_mask __ro_after_init;
 
 extern unsigned long ima_extended_pcrs_mask;
 
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 716bb302e75d..58829c98a69b 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -69,6 +69,12 @@ struct ima_algo_desc *ima_algo_array __ro_after_init;
 
 unsigned long ima_unsupported_pcr_banks_mask __ro_after_init;
 
+/*
+ * Subset of unsupported banks that had (possibly) been supported and
+ * maintained by some previous kernel in a kexec chain.
+ */
+unsigned long ima_discontinued_pcr_banks_mask __ro_after_init;
+
 static int __init ima_init_ima_crypto(void)
 {
 	long rc;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 83d5c7034919..4aa94584957e 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -156,7 +156,14 @@ static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 		return 0;
 
 #if !IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
-	pcr_banks_skip_mask = ima_unsupported_pcr_banks_mask;
+	/*
+	 * Don't invalidate unsupported PCR banks more than once in
+	 * general, with the exception of discontinued banks possibly
+	 * maintained by a previous kernel in a kexec chain and now in
+	 * indeterminate state.
+	 */
+	pcr_banks_skip_mask = ima_unsupported_pcr_banks_mask &
+		~ima_discontinued_pcr_banks_mask;
 	if (!(ima_extended_pcrs_mask & BIT(pcr))) {
 		/*
 		 * Invalidate unsupported banks once upon a PCR's
diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index 04c49f05cb74..4584cfc416f4 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -428,7 +428,9 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 	struct ima_template_desc *template_desc;
 	DECLARE_BITMAP(hdr_mask, HDR__LAST);
 	unsigned long count = 0;
-	int ret = 0;
+	unsigned long always_invalidated_pcrs_banks_mask = ~0UL;
+	unsigned long ever_invalidated_pcrs_banks_mask = 0;
+	int i, ret = 0;
 
 	if (!buf || size < sizeof(*khdr))
 		return 0;
@@ -527,10 +529,90 @@ int ima_restore_measurement_list(loff_t size, void *buf)
 
 		entry->pcr = !ima_canonical_fmt ? *(u32 *)(hdr[HDR_PCR].data) :
 			     le32_to_cpu(*(__le32 *)(hdr[HDR_PCR].data));
+		if (IMA_INVALID_PCR(entry->pcr)) {
+			pr_err("invalid measurement PCR index");
+			ret = -EINVAL;
+			break;
+		}
+
 		ret = ima_restore_measurement_entry(entry);
 		if (ret < 0)
 			break;
 
+#if !IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
+		if (ima_tpm_chip &&
+		    !(ima_extended_pcrs_mask & BIT(entry->pcr))) {
+			unsigned long invalidated_pcr_banks_mask;
+
+			invalidated_pcr_banks_mask =
+				ima_pcr_invalidated_banks(entry->pcr);
+			always_invalidated_pcrs_banks_mask &=
+				invalidated_pcr_banks_mask;
+			ever_invalidated_pcrs_banks_mask |=
+				invalidated_pcr_banks_mask;
+		}
+		ima_extended_pcrs_mask |= BIT(entry->pcr);
+#endif
 	}
+
+	always_invalidated_pcrs_banks_mask &= ever_invalidated_pcrs_banks_mask;
+	/*
+	 * Banks that had (possibly) been maintained by a previous
+	 * kernel in the kexec chain, but aren't anymore now, are
+	 * considered "discontinued" and in indeterminate
+	 * state. Re-invalidate unconditionally at each PCR extend.
+	 */
+	if (ima_extended_pcrs_mask != 0) {
+		ima_discontinued_pcr_banks_mask = ima_unsupported_pcr_banks_mask
+			& ~always_invalidated_pcrs_banks_mask;
+	}
+
+	/*
+	 * Banks consistently found invalidated for having been
+	 * unsupported before, are kept in the "unsupported" state and
+	 * never re-invalidated again, so that they retain their
+	 * easily recognizable, well-known digest value.
+	 */
+	ima_unsupported_pcr_banks_mask |= always_invalidated_pcrs_banks_mask;
+
+	for (i = 0; i < NR_BANKS(ima_tpm_chip); ++i) {
+		if (i >= BITS_PER_LONG)
+			break;
+
+		/*
+		 * Don't clutter the logs with unrecognized algos,
+		 * these would always get reported as "discontinued"
+		 * otherwise.
+		 */
+		if (ima_algo_array[i].algo == HASH_ALGO__LAST)
+			continue;
+
+		if (ima_discontinued_pcr_banks_mask & BIT(i)) {
+			pr_warn("Discontinuing unsupported TPM %s PCR bank\n",
+				hash_algo_name[ima_algo_array[i].algo]);
+		} else if (i == ima_hash_algo_idx &&
+			   ever_invalidated_pcrs_banks_mask & BIT(i)) {
+			pr_err("Continuing on stale ima_hash TPM PCR bank\n");
+
+			/* Force back to supported. */
+			ima_unsupported_pcr_banks_mask &= ~BIT(i);
+		} else if (always_invalidated_pcrs_banks_mask & BIT(i)) {
+			struct crypto_shash *tfm;
+
+			pr_info("Masking previously unsupported TPM %s PCR bank\n",
+				hash_algo_name[ima_algo_array[i].algo]);
+
+			/*
+			 * Remove the ->tfm so that any not yet
+			 * extended PCRs will get their corresponding
+			 * bank invalidated as well upon first use.
+			 */
+			tfm = ima_algo_array[i].tfm;
+			ima_algo_array[i].tfm = NULL;
+			if (tfm)
+				ima_free_tfm(tfm);
+		}
+	}
+
 	return ret;
 }
-- 
2.49.0

Re: [RFC PATCH v2 10/13] tpm: authenticate tpm2_pcr_read()

[James Bottomley]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> PCR reads aren't currently authenticated even with
> CONFIG_TCG_TPM2_HMAC=y yet.

The reason being TPM2_PCR_Read can only support an audit session, so it
has even more overhead than the usual HMAC session for something you
don't care about and because no-one relies on plain reads anyway,
relying entities use quotes.

> It is probably desirable though, as e.g. IMA does some PCR reads to
> form the cumulative boot digest subsequently extended into PCR 10 (an
> operation which *is* authenticated).

Could you elaborate on what security properties this adds?  I can't see
any form of attack that could be done by altering the boot aggregate:
either the relying party cares, in which case it will quote the boot
log and arrive at its own value, or it doesn't, in which case the value
in the log is superfluous.

> +		/*
> +		 * Exclusivity is not needed, but set in the
> response.
> +		 * Set it here too, so that the HMAC verification
> +		 * won't fail.
> +		 */
> +		tpm_buf_append_hmac_session(chip, &buf,
> TPM2_SA_AUDIT
> +					    |
> TPM2_SA_AUDIT_EXCLUSIVE,
> +					    NULL, 0);

Exclusivity here requires no other command be unaudited between the
session starting and now.  That means that with the lazy flush scheme
you have a reasonable chance of this being violated and triggering an
error on the command.

Additionally, the response will only have the exclusive flag set if the
above condition (no other unaudited command since session start) is
true, which it might not be.  The problem you're having is that
tpm2_auth_check_hmac_response() uses the command session flags to
calculate the rpHash, which is a useful short cut because for non-audit
sessions they're always the same.  If you want to use audit sessions,
you have to teach it to dig the response session flags out of the
header and use them instead.

Regards,

James

Re: [RFC PATCH v2 10/13] tpm: authenticate tpm2_pcr_read()

[Jarkko Sakkinen]

On Sun, Mar 23, 2025 at 03:09:08PM +0100, Nicolai Stange wrote:
> PCR reads aren't currently authenticated even with CONFIG_TCG_TPM2_HMAC=y
> yet.
> 
> It is probably desirable though, as e.g. IMA does some PCR reads to form
> the cumulative boot digest subsequently extended into PCR 10 (an operation
> which *is* authenticated).
> 
> Furthermore, a subsequent patch will make IMA to skip certain PCR bank
> re-invalidations (which are implemented with extensions) upon kexec based
> on the value read back at boot. In order to not weaken the overall
> security posture in this case, it will be required to establish the same
> level of trust into PCR reads as there is already for the extensions.
> 
> Make tpm2_pcr_read() to protect the command with a HMAC auth session,
> using the already existing infrastructure.
> 
> As the TPM2_PCR_Read command doesn't have any authorizations defined, and
> neither of TPM2_SA_ENCRYPT/TPM2_SA_DECRYPT is needed, use TPM2_SA_AUDIT,
> even though no auditing functionality is actually being required. Since
> the TPM will set TPM2_SA_AUDIT_EXCLUSIVE in its response with this
> single-use session, set it upfront so that tpm_buf_check_hmac_response()
> would expect it for the HMAC verification.
> 
> Now that tpm2_pcr_read() depends on the driver's session infrastructure,
> note that the first call to tpm2_pcr_read() at init time gets issued from
>   tpm_chip_bootstrap() -> tpm_get_pcr_allocation()
>   -> tpm2_get_pcr_allocation() -> tpm2_init_bank_info()
>   -> tpm2_pcr_read()
> after
>   tpm_chip_bootstrap() -> tpm_auto_startup() -> tpm2_auto_startup()
>   -> tpm2_sessions_init(),
> so there won't be any issues with that.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Please write a better commit message. There's extra words like 'yet'.

And e.g., subsequent patch means nothing in the commit log.  Please
don't use such terminology.

Not going to waste my life reading this.

BR, Jarkko 

Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Jarkko Sakkinen]

On Sun, Mar 23, 2025 at 03:09:05PM +0100, Nicolai Stange wrote:
> The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
> the corresponding digest from the provided digests[] argument.

Why not "just" tpm_pcr_extend(). We don't have a concept of
"non-existing tpm_pcr_extend()".

"tpm_pcr_extend() extends the allocated PCR banks ..."

or something.

> 
> An upcoming code change to IMA will introduce the need to skip over those

Don't talk about upcoming code changes. Just explain why IMA depends on
the change.

> banks it does not have a hash algorithm implementation available for.
> 
> Introduce tpm_pcr_extend_sel() to support this.
> 
> tpm_pcr_extend_sel() also expects a digests[] array, always being the
> number of allocated PCR banks in size, just as it's the case for the
> existing tpm_pcr_extend(). In addition to that however, it takes a
> 'banks_skip_mask', and will skip the extension of any bank having its
> corresponding bit set there.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
>  drivers/char/tpm/tpm.h           |  3 ++-
>  drivers/char/tpm/tpm2-cmd.c      | 29 +++++++++++++++++++++++++++--
>  include/linux/tpm.h              |  3 +++
>  4 files changed, 59 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b1daa0d7b341..88b4496de1df 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>   */
>  int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  		   struct tpm_digest *digests)
> +{
> +	return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
> +}
> +EXPORT_SYMBOL_GPL(tpm_pcr_extend);

I'd add just an extra argument to tpm_pcr_extend().

BR, Jarkko 

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[James Bottomley]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> Normally IMA would extend a template hash of each bank's associated
> algorithm into a PCR. However, if a bank's hash algorithm is
> unavailable to the kernel at IMA init time, it would fallback to
> extending padded SHA1 hashes instead.
> 
> That is, if e.g. SHA-256 was missing at IMA init, it would extend
> padded SHA1 template hashes into a PCR's SHA-256 bank.
> 
> The ima_measurement command (marked as experimental) from ima-evm-
> utils would accordingly try both variants when attempting to verify a
> measurement list against PCRs. keylime OTOH doesn't seem to -- it
> expects the template hash type to match the PCR bank algorithm. I
> would argue that for the latter case, the fallback scheme could
> potentially cause hard to debug verification failures.
> 
> There's another problem with the fallback scheme: right now, SHA-1
> availability is a hard requirement for IMA, and it would be good for
> a number of reasons to get rid of that. However, if SHA-1 is not
> available to the kernel, it can hardly provide padded SHA-1 template
> hashes for PCR banks with unsupported algos.

I think this was done against the day IMA only supported sha1 and the
TPM sha256 and beyond so there'd at least be a record that could be
replayed.  I think today with most distros defaulting IMAs hash to
sha256 that's much less of a problem.

> There are several more or less reasonable alternatives possible,
> among them are:
> a.) Instead of padded SHA-1, use padded/truncated ima_hash template
>     hashes.
> b.) Don't extend unsupported banks at all.
> c.) Record every event as a violation, i.e. extend unsupported banks
>     with 0xffs.
> d.) Invalidate unsupported banks at least once by extending with a
> unique
>     constant (e.g. with 0xfes).

Instead of any of that, why not do what the TCG tells us to do for
unsupported banks and simply cap them with 0xffffffff record
EV_SEPARATOR and stop extending to them? (note this would probably
require defining a separator event for IMA)

Regards,

James

Re: [RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Mimi Zohar]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> runtime_measurements_<hash-algo> sysfs files are getting created for
> each PCR bank + for SHA-1.
> 
> Now that runtime_measurements_<hash-algo> sysfs file creation is being
> skipped for unsupported hash algorithms, it will become possible that no
> such file would be provided at all once SHA-1 is made optional in a
> later patch.
> 
> Always create the file for the 'ima_hash' algorithm, even if it's not
> associated with any of the PCR banks. As IMA initialization will
> continue to fail if the ima_hash algorithm is not available to the
> kernel, this guarantees that at least one such file will always be
> there.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  security/integrity/ima/ima_fs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index a8df2fe5f4cb..f030ff7f56da 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void)
>  	u16 algo;
>  	int i;
>  
> -	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
> -
> -	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
> -		securityfs_measurement_list_count++;
> +	securityfs_measurement_list_count =
> +		NR_BANKS(ima_tpm_chip) + ima_extra_slots;
>  
>  	ascii_securityfs_measurement_lists =
>  	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),

"ima_hash" is the default file hash algorithm.  Re-using it as the default
complete measurement list assumes that the subsequent kexec'ed kernels configure
and define it as the default file hash algorithm as well, which might not be the
case.  Drop this patch.

Defer allocating the "extra" non-sha1 bank.  A subsequent patch will select
SHA256.  Based on the chosen algorithm, define the "extra" non-sha1 bank.

thanks,

Mimi

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Mimi Zohar]

> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 6f5696d999d0..a43080fb8edc 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -625,26 +625,43 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
>  	u16 alg_id;
>  	int rc, i;
>  
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
>  	if (rc)
>  		return rc;
>  
>  	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
> +#endif
>  
>  	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  		if (i == ima_sha1_idx)
>  			continue;
> +#endif
>  
>  		if (i < NR_BANKS(ima_tpm_chip)) {
>  			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
>  			entry->digests[i].alg_id = alg_id;
>  		}
>  
> -		/* for unmapped TPM algorithms digest is still a padded SHA1 */
> +		/*
> +		 * For unmapped TPM algorithms, the digest is still a
> +		 * padded SHA1 if backwards-compatibility fallback PCR
> +		 * extension is enabled. Otherwise fill with
> +		 * 0xfes. This is the value to invalidate unsupported
> +		 * PCR banks with. Also, a non-all-zeroes value serves
> +		 * as an indicator to kexec measurement restoration
> +		 * that the entry is not a violation and all its
> +		 * template digests need to get recomputed.
> +		 */
>  		if (!ima_algo_array[i].tfm) {
> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>  			memcpy(entry->digests[i].digest,
>  			       entry->digests[ima_sha1_idx].digest,
>  			       TPM_DIGEST_SIZE);
> +#else
> +			memset(entry->digests[i].digest, 0xfe, TPM_DIGEST_SIZE);
> +#endif

Using TPM_DIGEST_SIZE will result in a padded 0xfe value.

>  			continue;
>  		}
>  

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Mimi Zohar]

On Sun, 2025-03-23 at 17:18 -0400, James Bottomley wrote:
> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> > Normally IMA would extend a template hash of each bank's associated
> > algorithm into a PCR. However, if a bank's hash algorithm is
> > unavailable to the kernel at IMA init time, it would fallback to
> > extending padded SHA1 hashes instead.
> > 
> > That is, if e.g. SHA-256 was missing at IMA init, it would extend
> > padded SHA1 template hashes into a PCR's SHA-256 bank.
> > 
> > The ima_measurement command (marked as experimental) from ima-evm-
> > utils would accordingly try both variants when attempting to verify a
> > measurement list against PCRs. keylime OTOH doesn't seem to -- it
> > expects the template hash type to match the PCR bank algorithm. I
> > would argue that for the latter case, the fallback scheme could
> > potentially cause hard to debug verification failures.
> > 
> > There's another problem with the fallback scheme: right now, SHA-1
> > availability is a hard requirement for IMA, and it would be good for
> > a number of reasons to get rid of that. However, if SHA-1 is not
> > available to the kernel, it can hardly provide padded SHA-1 template
> > hashes for PCR banks with unsupported algos.
> 
> I think this was done against the day IMA only supported sha1 and the
> TPM sha256 and beyond so there'd at least be a record that could be
> replayed.  I think today with most distros defaulting IMAs hash to
> sha256 that's much less of a problem.

Commit 1ea973df6e21 ("ima: Calculate and extend PCR with digests in
ima_template_entry") added the support for extending multiple banks.  It
included the support for extending padded SHA1 digests for unknown TPM bank hash
algorithms.  Clearly it wasn't addressing the case of a TPM sha256 bank.

> 
> > There are several more or less reasonable alternatives possible,
> > among them are:
> > a.) Instead of padded SHA-1, use padded/truncated ima_hash template
> >     hashes.
> > b.) Don't extend unsupported banks at all.
> > c.) Record every event as a violation, i.e. extend unsupported banks
> >     with 0xffs.
> > d.) Invalidate unsupported banks at least once by extending with a
> > unique
> >     constant (e.g. with 0xfes).
> 
> Instead of any of that, why not do what the TCG tells us to do for
> unsupported banks and simply cap them with 0xffffffff record
> EV_SEPARATOR and stop extending to them? (note this would probably
> require defining a separator event for IMA)

open-writers and ToMToU integrity violations are added to the IMA measurement
list as 0x00's, but are extended into the TPM using 0xFF's.  Unfortunately, as
mentioned previously, some verifiers ignore these integrity violations by
automatically replacing the 0x00's with 0xFF's.

What do you mean by "simply cap" them?  Does it automatically prevent the PCR
from being extended?  If not, then this patch set is doing exactly that -
preventing the TPM bank from additional extends.

Mimi

Re: [RFC PATCH v2 01/13] ima: don't expose runtime_measurements for unsupported hashes

[Mimi Zohar]

On Sun, 2025-03-23 at 15:08 +0100, Nicolai Stange wrote:
> IMA creates one runtime_measurements_<hash-algo> sysfs file for every TPM
> bank + for SHA1 if not covered by any such. These differ only in that the
> template hash value for each record is of the file's associated algorithm
> each.
> 
> The kernel does not necessarily support each hash algorithm associated
> with some TPM bank though -- the most common case probably being that the
> algorithm is not built-in, but provided as a module, if at all, and thus
> not available at IMA init time yet.

At least from an IMA perspective, the IMA Kconfig selects "CRYPTO_SHA1", making
it the only crypto hash algorithm required to be built into the kernel.
> 
> If that happens to be the case, the behavior is a bit counter-intuitive:
> probably for historic reasons and to still extend the TPM bank with
> something, a record's template hash is filled with the padded SHA1 value.
> That is, it is perfectly possible that runtime_measurements_sha256 contains
> padded SHA1 template hashes if SHA-256 was unavailable at IMA init.

As you explained in "[RFC PATCH v2 09/13] ima: invalidate unsupported PCR banks
only once", not extending the TPM bank:
  b.) is a security risk, because the bank would validate an empty measurement
list.

The solution was to extend a padded SHA1 digest, which could still be verified.
The question boils down to whether extending the TPM bank with a valid, maybe
deprecated hash algo, is better than not extending it at all.  Was that the
right solution?  I believe it was at the time.  SHA1 was being deprecated for
specific use cases, not all usecases.

So the question is what to do going forward.

> 
> I would argue that it's likely that no existing userspace tool is relying
> on this fallback logic -- they either wouldn't consume the hash value from
> the measurement list directly but recreate it by themselves, as is required
> for verification against PCRs, or, if they did, they would somehow assume a
> hash algorithm and expect the hashes in the measurement list to be of that
> type. If of the latter kind, this could even lead to hard to debug
> verification failures. For example, from looking at keylime's current
> code, the verifier logic seems to assume that the template hashes found
> in the provided measurement list are of the configured 'ima_log_hash_alg'
> type. In particular, it does not check against padded SHA1 upon
> mismatch.

The downside, if none of the TPM bank hash algorithms are configured as builtin
in the kernel, is the lack of a measurement list.

True, not implementing the fallback is simpler, but it is their choice.

> 
> That being said, there's also another dimension: currently IMA has a
> hard requirement on SHA-1 and subsequent patches in this series will
> attempt to get rid of that. If SHA-1 is not available at IMA init though,
> it would also mean that padded SHA-1 values cannot get filled in as a
> fallback for other unsupported algorithms. Substituting something like
> hard coded all-zeroes or all-ones would be dangerous, because some
> application or user scripts could perhaps (ab)use the template hashes from
> the exported measurement lists for some kind of fingerprinting scheme or
> so.

Agreed, using the open-writer/ToMToU integrity violation to indicate an
unsupported TPM bank would not be a good idea.

> 
> In conclusion, I think it's best to not create the
> runtime_measurements_<hash-algo> sysfs files for hash algorithms not
> supported by the kernel. That way, applications expecting a certain
> hash algorithm for the measurement list and which are not able to handle
> the padded-SHA1 fallback scheme would fail with a clear indication on what
> the proem is. Furthermore, as digests for unsupported banks are not
> getting exposed to userspace anymore, we'll have all flexibility to
> set it to any value internally, including all-ones as will be needed in
> a subsequent patch when addressing PCR extend for unsupported banks.
> 
> So, do not create runtime_measurements_<hash-algo> sysfs files for
> unsupported hash algorithms. Likewise for their ascii counterparts.
> 
> Note that at this point, SHA-1 is still mandatory, and thus,
> runtime_measurements_sha1 as well as the "runtime_measurements" will
> remain there, even though the code has provisions already to skip their
> creation as well in case SHA-1 was unavailable.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> 

If the purpose of this patch set is to actually remove IMA's dependency on a
working SHA-1, at some point the Kconfig "select CRYPTO_SHA1" needs to be
removed.  Otherwise the kernel will be built with SHA1 builtin
(CONFIG_CRYPTO_SHA1=y).

If the purpose of this patch set is preparatory for eventually removing the SHA1
dependency, then the cover letter and the patch descriptions should indicate
that.

Assuming the latter, other than updating the patch description, the patch is
fine.

Before posting the non-RFC version of this patch, please trim the patch
description.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---
>  security/integrity/ima/ima_fs.c | 35 +++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index e4a79a9b2d58..a8df2fe5f4cb 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -454,6 +454,9 @@ static int __init create_securityfs_measurement_lists(void)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < securityfs_measurement_list_count; i++) {
> +		if (!ima_algo_array[i].tfm)
> +			continue;
> +
>  		algo = ima_algo_array[i].algo;
>  
>  		sprintf(file_name, "ascii_runtime_measurements_%s",
> @@ -573,20 +576,26 @@ int __init ima_fs_init(void)
>  	if (ret != 0)
>  		goto out;
>  
> -	binary_runtime_measurements =
> -	    securityfs_create_symlink("binary_runtime_measurements", ima_dir,
> -				      "binary_runtime_measurements_sha1", NULL);
> -	if (IS_ERR(binary_runtime_measurements)) {
> -		ret = PTR_ERR(binary_runtime_measurements);
> -		goto out;
> -	}
> +	if (ima_algo_array[ima_sha1_idx].tfm) {
> +		binary_runtime_measurements =
> +		    securityfs_create_symlink("binary_runtime_measurements",
> +					      ima_dir,
> +					      "binary_runtime_measurements_sha1",
> +					      NULL);
> +		if (IS_ERR(binary_runtime_measurements)) {
> +			ret = PTR_ERR(binary_runtime_measurements);
> +			goto out;
> +		}
>  
> -	ascii_runtime_measurements =
> -	    securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
> -				      "ascii_runtime_measurements_sha1", NULL);
> -	if (IS_ERR(ascii_runtime_measurements)) {
> -		ret = PTR_ERR(ascii_runtime_measurements);
> -		goto out;
> +		ascii_runtime_measurements =
> +		    securityfs_create_symlink("ascii_runtime_measurements",
> +					      ima_dir,
> +					      "ascii_runtime_measurements_sha1",
> +					      NULL);
> +		if (IS_ERR(ascii_runtime_measurements)) {
> +			ret = PTR_ERR(ascii_runtime_measurements);
> +			goto out;
> +		}
>  	}
>  
>  	runtime_measurements_count =

Re: [RFC PATCH v2 05/13] ima: select CRYPTO_SHA256 from Kconfig

[Mimi Zohar]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> Since recently, IMA would not record measurement list entries into PCR
> banks for which it doesn't have a corresponding in-kernel hash algorithm
> implementation available anymore (for
> CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND=n).

Not necessary info.

> 
> With TPM 2.0, the only hash algorithms guaranteed to be implemented on a
> TPM are SHA-256/384, c.f. "TCG PC Client Platform TPM Profile
> Specification for TPM 2.0", sec. 4.6 "PCR Requirements".

Ok

> In particular, sha1 is not mandatory, and thus, the CRYPTO_SHA1 dependency
> of IMA is not sufficient anymore for ensuring that IMA would find at least
> one usable PCR bank.

No necessary info.

> 
> So, in order to make sure that IMA has access to at least one usable bank
> on platforms featuring a TPM 2.0 device, make it depend on CRYPTO_SHA256.

-> Make sure that ...

> 
> Keep the dependency on CRYPTO_SHA1 for the TPM 1 case.

Wondering if the "select CRYPTO_SHA1" could be dependent on TPM 1.2 being
configured as builtin.

> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>
> ---
>  security/integrity/ima/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index c8f12a4a4edf..8a7e74dc1477 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -7,6 +7,7 @@ config IMA
>  	select CRYPTO
>  	select CRYPTO_HMAC
>  	select CRYPTO_SHA1
> +	select CRYPTO_SHA256
>  	select CRYPTO_HASH_INFO
>  	select SECURITY_PATH
>  	select TCG_TPM if HAS_IOMEM

It's not enough to "select CRYPTO_SHA256".  As mentioned on "[RFC PATCH v2
02/13] ima: always create runtime_measurements sysfs file for ima_hash",  don't
assume "ima_hash" will be SHA256.  Include SHA256 as an "extra" hash algorithm,
even if it isn't an enabled TPM bank.

Mimi

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[James Bottomley]

On Mon, 2025-03-24 at 21:03 -0400, Mimi Zohar wrote:
> On Sun, 2025-03-23 at 17:18 -0400, James Bottomley wrote:
[...]
> > Instead of any of that, why not do what the TCG tells us to do for
> > unsupported banks and simply cap them with 0xffffffff record
> > EV_SEPARATOR and stop extending to them? (note this would probably
> > require defining a separator event for IMA)
> 
> open-writers and ToMToU integrity violations are added to the IMA
> measurement list as 0x00's, but are extended into the TPM using
> 0xFF's.  Unfortunately, as mentioned previously, some verifiers
> ignore these integrity violations by automatically replacing the
> 0x00's with 0xFF's.

That sounds like something that should be fixed ...

> What do you mean by "simply cap" them?  Does it automatically prevent
> the PCR from being extended?  If not, then this patch set is doing
> exactly that - preventing the TPM bank from additional extends.

The idea of separators as understood by the TCG (the EV_SEPARATOR
event) is that they divide the log up into different phases.  If you
see a measurement belonging to a prior phase after a separator you know
some violation has occurred, even if the log itself verifies.  The
point being that if you log a separator in the last phase of boot (and
for IMA logs there only is a single phase) there can be no more valid
measurements after that event because of the separator, so the PCR is
termed capped, meaning you can't validly extend to it and if you do the
verifier shows a violation.

Regards,

James

Re: [RFC PATCH v2 08/13] ima: track the set of PCRs ever extended

[Mimi Zohar]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> Right now, PCR banks with unsupported hash algorithms are getting
> invalidated over and over again for each new measurement list entry
> recorded.
> 
> A subsequent patch will make IMA to invalidate PCR banks associated with
> unsupported hash algorithms only once at a PCR's first use. To prepare for
> that, make it track the set of PCRs ever extended.
> 
> Maintain the set of touched PCRs in an unsigned long bitmask,
> 'ima_extended_pcrs_mask'.
> 
> Amend the IMA_INVALID_PCR() #define to check that a given PCR can get
> represented in that bitmask. Note that this is only for improving code
> maintainablity, it does not actually constain the set of allowed PCR
> indices any further.
> 
> Make ima_pcr_extend() to maintain the ima_extended_pcrs_mask, i.e. to set
> the currently extented PCR's corresponding bit.
> 
> Note that at this point there's no provision to restore the
> ima_extended_pcrs_mask value after kexecs yet, that will be the subject of
> later patches.
> 
> Signed-off-by: Nicolai Stange <nstange@suse.de>

Hi Nicolai,

IMA extends measurements in the default TPM PCR based on the Kconfig
CONFIG_IMA_MEASURE_PCR_IDX option.  Normally that is set to PCR 10.  The IMA
policy rules may override the default PCR with a per policy rule specific PCR.

INVALID_PCR() checks the IMA policy rule specified is a valid PCR register.

Is the purpose of this patch to have a single per TPM bank violation or multiple
violations, one for each PCR used within the TPM bank?

thanks,

Mimi

> ---
>  security/integrity/ima/ima.h       |  8 ++++++--
>  security/integrity/ima/ima_queue.c | 17 +++++++++++++----
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 1158a7b8bf6b..f99b1f81b35c 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -20,6 +20,7 @@
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
>  #include <linux/audit.h>
> +#include <linux/minmax.h>
>  #include <crypto/hash_info.h>
>  
>  #include "../integrity.h"
> @@ -62,6 +63,8 @@ extern int ima_hash_algo_idx __ro_after_init;
>  extern int ima_extra_slots __ro_after_init;
>  extern struct ima_algo_desc *ima_algo_array __ro_after_init;
>  
> +extern unsigned long ima_extended_pcrs_mask;
> +
>  extern int ima_appraise;
>  extern struct tpm_chip *ima_tpm_chip;
>  extern const char boot_aggregate_name[];
> @@ -198,8 +201,9 @@ struct ima_iint_cache {
>  	struct ima_digest_data *ima_hash;
>  };
>  
> -#define IMA_INVALID_PCR(a) (((a) < 0) || \
> -	(a) >= (sizeof_field(struct ima_iint_cache, measured_pcrs) * 8))
> +#define IMA_INVALID_PCR(a) (((a) < 0) ||				    \
> +	(a) >= (8 * min(sizeof_field(struct ima_iint_cache, measured_pcrs), \
> +			sizeof(ima_extended_pcrs_mask))))
>  
>  
>  extern struct lsm_blob_sizes ima_blob_sizes;
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index 0cc1189446a8..6e8a7514d9f6 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -51,6 +51,11 @@ static DEFINE_MUTEX(ima_extend_list_mutex);
>   */
>  static bool ima_measurements_suspended;
>  
> +/*
> + * Set of PCRs ever extended by IMA.
> + */
> +unsigned long ima_extended_pcrs_mask;
> +
>  /* lookup up the digest value in the hash table, and return the entry */
>  static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
>  						       int pcr)
> @@ -144,15 +149,19 @@ unsigned long ima_get_binary_runtime_size(void)
>  
>  static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
>  {
> -	int result = 0;
> +	int result;
>  
>  	if (!ima_tpm_chip)
> -		return result;
> +		return 0;
>  
>  	result = tpm_pcr_extend(ima_tpm_chip, pcr, digests_arg);
> -	if (result != 0)
> +	if (result != 0) {
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
> -	return result;
> +		return result;
> +	}
> +
> +	ima_extended_pcrs_mask |= BIT(pcr);
> +	return 0;
>  }
>  
>  /*

Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Mimi Zohar]

On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:

> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index dfdcbd009720..23ded8ea47dc 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>   * @chip:	TPM chip to use.
>   * @pcr_idx:	index of the PCR.
>   * @digests:	list of pcr banks and corresponding digest values to extend.
> + * @banks_skip_mask:	pcr banks to skip
>   *
>   * Return: Same as with tpm_transmit_cmd.
>   */
>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> -		    struct tpm_digest *digests)
> +		    struct tpm_digest *digests,
> +		    unsigned long banks_skip_mask)
>  {
>  	struct tpm_buf buf;
> +	unsigned long skip_mask;
> +	u32 banks_count;
>  	int rc;
>  	int i;
>  
> +	banks_count = 0;
> +	skip_mask = banks_skip_mask;
> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
> +		const bool skip_bank = skip_mask & 1;
> +
> +		skip_mask >>= 1;
> +		if (skip_bank)
> +			continue;
> +		banks_count++;
> +	}

Setting ima_unsupported_pcr_banks_mask used BIT(i).  Testing the bit should be
as straight forward here and below.

The first TPM extend after boot is the boot_aggregate.  Afterwards the number of
banks being extended should always be the same.  Do we really need to re-
calculate the number of banks needing to be extended each time?

> +
> +	if (banks_count == 0)
> +		return 0;
> +
>  	if (!disable_pcr_integrity) {
>  		rc = tpm2_start_auth_session(chip);
>  		if (rc)
> @@ -257,9 +275,16 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
>  	}
>  
> -	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> +	tpm_buf_append_u32(&buf, banks_count);
>  
> +	skip_mask = banks_skip_mask;
>  	for (i = 0; i < chip->nr_allocated_banks; i++) {
> +		const bool skip_bank = skip_mask & 1;
> +
> +		skip_mask >>= 1;
> +		if (skip_bank)
> +			continue;
> +

>  		tpm_buf_append_u16(&buf, digests[i].alg_id);
>  		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
>  			       chip->allocated_banks[i].digest_size);

Re: [RFC PATCH v2 00/13] ima: get rid of hard dependency on SHA-1

[Mimi Zohar]

On Sun, 2025-03-23 at 15:08 +0100, Nicolai Stange wrote:
> Hi,
> 
> this is v2 of the RFC series to disentangle IMA from its current
> dependency on a working SHA-1 implementation.
> 
> For reference, v1 can be found at [1].
> 
> Several options for when and how to invalidate unsupported TPM PCR banks
> by extending them with a unique constant had been discussed at the v1:
> a.) every single time a new entry gets added to the measurement list
> b.) or only once.
> 
> b.) is appealing, because it enables recognizing unsupported banks right
> away from their value, but comes at a significant additional complexity.
> Fortunately, it turned out that it's possible to develop b.) incrementally
> on top of a.), so this series can get truncated
> - after [5/13] ("ima: select CRYPTO_SHA256 from Kconfig") to get a.),
> - or after [9/13] ("ima: invalidate unsupported PCR banks only once")
>   to get a partial b.), invalidating unsupported banks only once for
>   each kernel booted, but redoing it for each kernel in a kexec chain,
> - or not at all to get the full b.), i.e. to skip reinvalidations even
>   from later kernels in the kexec chain if possible.
> 
> I would personally go for the full set, because it also enables some
> perhaps helpful diagnostics for the kernel log, but OTOH I'm clearly
> biased now because I've implemented everthing. So it's your judgement
> call now on how to proceed. Either way, I would send the next iteration in
> non-RFC mode with the full CC set. If you opted for a.) only, it would be
> a.) only, i.e. [1-5/13]. If you decided for b.), it might make sense to
> send in two batches to facilitate review: [1-9/13] first and the rest
> somewhen later.

Agreed.  Reviewing 1-9/13 is plenty.

To summarize/re-iterate:

- Don't rely on the "ima_hash" algorithm being the same for the kexec'ed kernel.
Create an "extra" bank for sha256, if the TPM is not configured for it.  We'll
be guaranteed at least one complete measurement list.

- Testing without SHA1 is difficult.  Maybe "select CRYPTO_SHA1" could be made
dependent on TPM 1.2 being enabled.

- From a testing perspective, it might be simpler to define a new Kconfig, but
I'm not sure it is really needed.  If SHA1 is configured as builtin, then
continue extending the unsupported TPM banks with SHA1.  Only if SHA1 isn't
configured, extend a single invalidate record (0xFE) per unsupported TPM bank.

- Should multiple PCRs be invalidated if the TPM bank hash algorithm was not
configured as builtin?  Or just invalidate the CONFIG_IMA_MEASURE_PCR_IDX?

- The number of unsupported TPM banks doesn't change until the next kexec.  No
need to keep re-calculating the number of TPM banks to extend.

Mimi

> 
> FWIW, I did some testing now, on the full series in a VM with a swtpm
> attached to it:
> - both with and without CONFIG_TCG_TPM2_HMAC (for [10/13] ("tpm:
>   authenticate tpm2_pcr_read()" coverage) and
> - with a focus on verifying everything related to the new invalidation
>   logic is working as intended.
> 
> Thanks a lot!
> 
> Nicolai
> 
> 
> 
> Changes to v1:
> - [v1 1/7] ("ima: don't expose runtime_measurements for unsupported
>   hashes"): no change.
> - [v1 2/7] ("ima: always create runtime_measurements sysfs file for
>   ima_hash"): no change.
> - [v1 3/7] ("ima: move INVALID_PCR() to ima.h"): moved to [v2 6/13],
>   otherwise no change.
> - [v1 4/7] ("ima: track the set of PCRs ever extended"):
>   moved to [v2 8/13], drop code restoring ima_extended_pcrs_mask at kexec,
>   update it from ima_pcr_extend() only if the tpm_pcr_extend() was
>   successful.
> - [v1 5/7] ("tpm: enable bank selection for PCR extend"): moved to
>   [v2 7/13], fix a bug by actually passing the skip mask from
>   tpm_pcr_extend() to tpm2_pcr_extend().
> - [v1 6/7] ("ima: invalidate unsupported PCR banks once at first use"):
>   gone, superseded by the new
>     [v2 3/13]  ("invalidate unsupported PCR banks")
>     [v2 9/13]  ("ima: invalidate unsupported PCR banks only once")
>     [v2 13/13] ("ima: don't re-invalidate unsupported PCR banks after
>               kexec")
> - [v1 7/7] ("ima: make SHA1 non-mandatory"): moved to [v2 4/13],
>   diff context updates due to ima_unsupported_tpm_banks_mask not
>   existing yet at this point in the series.
> 
> - [v2 5/13] ("ima: select CRYPTO_SHA256 from Kconfig"): new to
>   (hopefully) address feedback at [2].
> - [v2 10/13] ("tpm: authenticate tpm2_pcr_read()"): new, prerequisite
>   for the next in a sense.
> - [v2 11/13] ("ima: introduce ima_pcr_invalidated_banks() helper"): new,
>   prerequisite for [13/13].
> - [v2 12/13] ("ma: make ima_free_tfm()'s linkage extern"): new,
>   likewise a prerequisite for [13/13].
> 
> 
> [1] https://lore.kernel.org/r/20250313173339.3815589-1-nstange@suse.de
> [2] https://lore.kernel.org/r/4e760360258bda56fbcb8f67e865a7a4574c305a.camel@linux.ibm.com
> 
> 
> Nicolai Stange (13):
>   ima: don't expose runtime_measurements for unsupported hashes
>   ima: always create runtime_measurements sysfs file for ima_hash
>   ima: invalidate unsupported PCR banks
>   ima: make SHA1 non-mandatory
>   ima: select CRYPTO_SHA256 from Kconfig
>   ima: move INVALID_PCR() to ima.h
>   tpm: enable bank selection for PCR extend
>   ima: track the set of PCRs ever extended
>   ima: invalidate unsupported PCR banks only once
>   tpm: authenticate tpm2_pcr_read()
>   ima: introduce ima_pcr_invalidated_banks() helper
>   ima: make ima_free_tfm()'s linkage extern
>   ima: don't re-invalidate unsupported PCR banks after kexec
> 
>  drivers/char/tpm/tpm-interface.c      |  29 +++-
>  drivers/char/tpm/tpm.h                |   3 +-
>  drivers/char/tpm/tpm2-cmd.c           |  75 ++++++++-
>  include/linux/tpm.h                   |   3 +
>  security/integrity/ima/Kconfig        |  15 ++
>  security/integrity/ima/ima.h          |  12 ++
>  security/integrity/ima/ima_crypto.c   | 216 ++++++++++++++++++++++----
>  security/integrity/ima/ima_fs.c       |  41 +++--
>  security/integrity/ima/ima_policy.c   |   5 +-
>  security/integrity/ima/ima_queue.c    |  54 ++++++-
>  security/integrity/ima/ima_template.c |  84 +++++++++-
>  11 files changed, 471 insertions(+), 66 deletions(-)
> 

Re: [RFC PATCH v2 10/13] tpm: authenticate tpm2_pcr_read()

[Nicolai Stange]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>> PCR reads aren't currently authenticated even with
>> CONFIG_TCG_TPM2_HMAC=y yet.
>
> The reason being TPM2_PCR_Read can only support an audit session, so it
> has even more overhead than the usual HMAC session for something you
> don't care about and because no-one relies on plain reads anyway,
> relying entities use quotes.
>
>> It is probably desirable though, as e.g. IMA does some PCR reads to
>> form the cumulative boot digest subsequently extended into PCR 10 (an
>> operation which *is* authenticated).
>
> Could you elaborate on what security properties this adds?  I can't see
> any form of attack that could be done by altering the boot aggregate:
> either the relying party cares, in which case it will quote the boot
> log and arrive at its own value, or it doesn't, in which case the value
> in the log is superfluous.

Thanks a lot for the explanation, it makes a lot of sense now. The above
was assumption based, along the lines of "the boot_aggregate gets
measured into the IMA PCR, therefore committing to its value must serve
some purpose and the extended value should probably be genuine".

I would like to make it clear that my main motivation for this patch
wasn't about the IMA-measured boot_aggregate integrity, but more about
not getting blamed for (silently) breaking the null_key auth based
protection guarantees provided for IMA's PCR extends with the last patch
in this series (which would skip some extends conditioned on what's
previously been read). FWIW, it's been agreed upon to split this series
in batches, with the first one extending to up to [9/13] only, so this
patch here wouldn't be part of that.

>> +		/*
>> +		 * Exclusivity is not needed, but set in the
>> response.
>> +		 * Set it here too, so that the HMAC verification
>> +		 * won't fail.
>> +		 */
>> +		tpm_buf_append_hmac_session(chip, &buf,
>> TPM2_SA_AUDIT
>> +					    |
>> TPM2_SA_AUDIT_EXCLUSIVE,
>> +					    NULL, 0);
>
> Exclusivity here requires no other command be unaudited between the
> session starting and now.  That means that with the lazy flush scheme
> you have a reasonable chance of this being violated and triggering an
> error on the command.

Noted, I didn't realize there's a lazy flushing scheme in place.

Thanks!

Nicolai

>
> Additionally, the response will only have the exclusive flag set if the
> above condition (no other unaudited command since session start) is
> true, which it might not be.  The problem you're having is that
> tpm2_auth_check_hmac_response() uses the command session flags to
> calculate the rpHash, which is a useful short cut because for non-audit
> sessions they're always the same.  If you want to use audit sessions,
> you have to teach it to dig the response session flags out of the
> header and use them instead.
>

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 01/13] ima: don't expose runtime_measurements for unsupported hashes

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Sun, 2025-03-23 at 15:08 +0100, Nicolai Stange wrote:
>> IMA creates one runtime_measurements_<hash-algo> sysfs file for every TPM
>> bank + for SHA1 if not covered by any such. These differ only in that the
>> template hash value for each record is of the file's associated algorithm
>> each.
>> 
>> The kernel does not necessarily support each hash algorithm associated
>> with some TPM bank though -- the most common case probably being that the
>> algorithm is not built-in, but provided as a module, if at all, and thus
>> not available at IMA init time yet.
>
> At least from an IMA perspective, the IMA Kconfig selects "CRYPTO_SHA1", making
> it the only crypto hash algorithm required to be built into the kernel.
>> 
>> If that happens to be the case, the behavior is a bit counter-intuitive:
>> probably for historic reasons and to still extend the TPM bank with
>> something, a record's template hash is filled with the padded SHA1 value.
>> That is, it is perfectly possible that runtime_measurements_sha256 contains
>> padded SHA1 template hashes if SHA-256 was unavailable at IMA init.
>
> As you explained in "[RFC PATCH v2 09/13] ima: invalidate unsupported PCR banks
> only once", not extending the TPM bank:
>   b.) is a security risk, because the bank would validate an empty measurement
> list.
>
> The solution was to extend a padded SHA1 digest, which could still be verified.
> The question boils down to whether extending the TPM bank with a valid, maybe
> deprecated hash algo, is better than not extending it at all.  Was that the
> right solution?  I believe it was at the time.  SHA1 was being deprecated for
> specific use cases, not all usecases.

No doubt about that, I'll drop the "counter-intuitive" part. Even now
(or by 2030 to be more specific), it should be only FIPS users caring
about SHA1 in this context at all.


> So the question is what to do going forward.
>
>> 
>> I would argue that it's likely that no existing userspace tool is relying
>> on this fallback logic -- they either wouldn't consume the hash value from
>> the measurement list directly but recreate it by themselves, as is required
>> for verification against PCRs, or, if they did, they would somehow assume a
>> hash algorithm and expect the hashes in the measurement list to be of that
>> type. If of the latter kind, this could even lead to hard to debug
>> verification failures. For example, from looking at keylime's current
>> code, the verifier logic seems to assume that the template hashes found
>> in the provided measurement list are of the configured 'ima_log_hash_alg'
>> type. In particular, it does not check against padded SHA1 upon
>> mismatch.
>
> The downside, if none of the TPM bank hash algorithms are configured as builtin
> in the kernel, is the lack of a measurement list.

Yes. Just for the record, going forward SHA256 will be, with [v2 05/13]. So
unless the SHA256 bank is disabled by firmware, it should be fine then.


> True, not implementing the fallback is simpler, but it is their
> choice.

The point I was trying to make is that no tool currently exists that
would successfully verify a /sys/*/*runtime_measurements_sha256 with
SHA1s template hashes in it. keylime would not as outlined above, and
ima-evm-utils ima_measurement command wouldn't either, because it works
only on /sys/*/*runtime_measurements_sha1 to begin with. (I previously
missed the latter fact, but its ima_verify_template_hash() does a
hard-coded SHA1). That is, ima_measurement's fallback logic only
applies to the PCR validations, not to the sysfs files.


>> That being said, there's also another dimension: currently IMA has a
>> hard requirement on SHA-1 and subsequent patches in this series will
>> attempt to get rid of that. If SHA-1 is not available at IMA init though,
>> it would also mean that padded SHA-1 values cannot get filled in as a
>> fallback for other unsupported algorithms. Substituting something like
>> hard coded all-zeroes or all-ones would be dangerous, because some
>> application or user scripts could perhaps (ab)use the template hashes from
>> the exported measurement lists for some kind of fingerprinting scheme or
>> so.
>
> Agreed, using the open-writer/ToMToU integrity violation to indicate an
> unsupported TPM bank would not be a good idea.
>
>> 
>> In conclusion, I think it's best to not create the
>> runtime_measurements_<hash-algo> sysfs files for hash algorithms not
>> supported by the kernel. That way, applications expecting a certain
>> hash algorithm for the measurement list and which are not able to handle
>> the padded-SHA1 fallback scheme would fail with a clear indication on what
>> the proem is. Furthermore, as digests for unsupported banks are not
>> getting exposed to userspace anymore, we'll have all flexibility to
>> set it to any value internally, including all-ones as will be needed in
>> a subsequent patch when addressing PCR extend for unsupported banks.
>> 
>> So, do not create runtime_measurements_<hash-algo> sysfs files for
>> unsupported hash algorithms. Likewise for their ascii counterparts.
>> 
>> Note that at this point, SHA-1 is still mandatory, and thus,
>> runtime_measurements_sha1 as well as the "runtime_measurements" will
>> remain there, even though the code has provisions already to skip their
>> creation as well in case SHA-1 was unavailable.
>> 
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> 
>
> If the purpose of this patch set is to actually remove IMA's dependency on a
> working SHA-1, at some point the Kconfig "select CRYPTO_SHA1" needs to be
> removed.  Otherwise the kernel will be built with SHA1 builtin
> (CONFIG_CRYPTO_SHA1=y).

I should have described it better. In the first step at least, the goal
is to remove the runtime dependency only. Because when SHA1's
->fips_allowed in crypto/testmgr.c gets flipped to false, SHA1
instantiation would fail with -ENOENT if the kernel was booted with a
fips=1 on its command line. Users not interested in FIPS, i.e. the vast
majority, might still want to use SHA1 and there's no real reason not
to.

But yes, it would definitely make sense to drop the CRYPTO_SHA1 dep, at
least at some point. Perhaps by simply moving it to the new
IMA_COMPAT_FALLBACK_TPM_EXTEND. I would personally not do that now
though, just in case there'll be some unexpected fallout from this
series.


> If the purpose of this patch set is preparatory for eventually removing the SHA1
> dependency, then the cover letter and the patch descriptions should indicate
> that.
>
> Assuming the latter, other than updating the patch description, the patch is
> fine.
>
> Before posting the non-RFC version of this patch, please trim the patch
> description.

Point taken.


> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Thanks _a lot_!

Nicolai

>
-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>> runtime_measurements_<hash-algo> sysfs files are getting created for
>> each PCR bank + for SHA-1.
>> 
>> Now that runtime_measurements_<hash-algo> sysfs file creation is being
>> skipped for unsupported hash algorithms, it will become possible that no
>> such file would be provided at all once SHA-1 is made optional in a
>> later patch.
>> 
>> Always create the file for the 'ima_hash' algorithm, even if it's not
>> associated with any of the PCR banks. As IMA initialization will
>> continue to fail if the ima_hash algorithm is not available to the
>> kernel, this guarantees that at least one such file will always be
>> there.
>> 
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> ---
>>  security/integrity/ima/ima_fs.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index a8df2fe5f4cb..f030ff7f56da 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void)
>>  	u16 algo;
>>  	int i;
>>  
>> -	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
>> -
>> -	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
>> -		securityfs_measurement_list_count++;
>> +	securityfs_measurement_list_count =
>> +		NR_BANKS(ima_tpm_chip) + ima_extra_slots;
>>  
>>  	ascii_securityfs_measurement_lists =
>>  	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
>
> "ima_hash" is the default file hash algorithm.  Re-using it as the default
> complete measurement list assumes that the subsequent kexec'ed kernels configure
> and define it as the default file hash algorithm as well, which might not be the
> case.

I don't really see why the ima_hashes would have to match between kexecs
for this to work -- all events' template hashes are getting recreated
from scratch anyway after kexec (ima_restore_measurement_list() ->
ima_calc_field_array_hash()).

That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one
would have *runtime_measurements_sha256 first and
*runtime_measurements_sha384 after kexec. And both had exclusively
template hashes of their respective algo in them each.

What am I missing?


> Drop this patch.

Fine by me, but just to confirm, in case there's no TPM attached and
SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all
then. Is that Ok?

ima_hash was chosen here only, because after this series, it will be the
only single algorithm guaranteed to be available.

Thanks!

Nicolai


> Defer allocating the "extra" non-sha1 bank.  A subsequent patch will select
> SHA256.  Based on the chosen algorithm, define the "extra" non-sha1 bank.
>
-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Nicolai Stange]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Mon, 2025-03-24 at 21:03 -0400, Mimi Zohar wrote:
>> On Sun, 2025-03-23 at 17:18 -0400, James Bottomley wrote:
> [...]
>> > Instead of any of that, why not do what the TCG tells us to do for
>> > unsupported banks and simply cap them with 0xffffffff record
>> > EV_SEPARATOR and stop extending to them? (note this would probably
>> > require defining a separator event for IMA)
>> 
>> open-writers and ToMToU integrity violations are added to the IMA
>> measurement list as 0x00's, but are extended into the TPM using
>> 0xFF's.  Unfortunately, as mentioned previously, some verifiers
>> ignore these integrity violations by automatically replacing the
>> 0x00's with 0xFF's.

I've researched the EV_SEPARATOR now, and according to [1], sec. 10.4.1
("Event Types"), PDF p. 128, the _digest_ of 0xffffffff is to get
extended. So there's no conflict with how IMA violations are extended
(plain 0xff ... ff), in case that was the reason Mimi mentioned it.

However, the main point of this patchset is to handle unsupported algos,
so I think the HASH(0xffffffff) constant cannot get computed.


> That sounds like something that should be fixed ...
>
>> What do you mean by "simply cap" them?  Does it automatically prevent
>> the PCR from being extended?  If not, then this patch set is doing
>> exactly that - preventing the TPM bank from additional extends.
>
> The idea of separators as understood by the TCG (the EV_SEPARATOR
> event) is that they divide the log up into different phases.  If you
> see a measurement belonging to a prior phase after a separator you know
> some violation has occurred, even if the log itself verifies.  The
> point being that if you log a separator in the last phase of boot (and
> for IMA logs there only is a single phase) there can be no more valid
> measurements after that event because of the separator, so the PCR is
> termed capped, meaning you can't validly extend to it and if you do the
> verifier shows a violation.

The motivation for extending with constant 0xfe ... fe into unsupported
banks is based on a very similar line of reasoning: because no event
template HASH() would possibly come out as that particular constant, no
sequence of events, including an empty one, could get verified against
such a bank.

Thanks,

Nicolai

[1] TCG PC Client Platform Firmware Profile Specification, Level 00
    Version 1.06 Revision 52, December 4, 2023
    https://trustedcomputinggroup.org/wp-content/uploads/TCG-PC-Client-Platform-Firmware-Profile-Version-1.06-Revision-52_pub-3.pdf

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

>> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> index 6f5696d999d0..a43080fb8edc 100644
>> --- a/security/integrity/ima/ima_crypto.c
>> +++ b/security/integrity/ima/ima_crypto.c
>> @@ -625,26 +625,43 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
>>  	u16 alg_id;
>>  	int rc, i;
>>  
>> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>>  	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
>>  	if (rc)
>>  		return rc;
>>  
>>  	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
>> +#endif
>>  
>>  	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
>> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>>  		if (i == ima_sha1_idx)
>>  			continue;
>> +#endif
>>  
>>  		if (i < NR_BANKS(ima_tpm_chip)) {
>>  			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
>>  			entry->digests[i].alg_id = alg_id;
>>  		}
>>  
>> -		/* for unmapped TPM algorithms digest is still a padded SHA1 */
>> +		/*
>> +		 * For unmapped TPM algorithms, the digest is still a
>> +		 * padded SHA1 if backwards-compatibility fallback PCR
>> +		 * extension is enabled. Otherwise fill with
>> +		 * 0xfes. This is the value to invalidate unsupported
>> +		 * PCR banks with. Also, a non-all-zeroes value serves
>> +		 * as an indicator to kexec measurement restoration
>> +		 * that the entry is not a violation and all its
>> +		 * template digests need to get recomputed.
>> +		 */
>>  		if (!ima_algo_array[i].tfm) {
>> +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>>  			memcpy(entry->digests[i].digest,
>>  			       entry->digests[ima_sha1_idx].digest,
>>  			       TPM_DIGEST_SIZE);

                               ^
That's been here before, just for the record for the below.

>> +#else
>> +			memset(entry->digests[i].digest, 0xfe, TPM_DIGEST_SIZE);
>> +#endif
>
> Using TPM_DIGEST_SIZE will result in a padded 0xfe value.

Yes, but as the sysfs files for unsupported algos are gone, this will be
used only for extending the PCR banks. tpm[12]_pcr_extend()
(necessarily) truncate the digests to the correct size before sending
them to the TPM.

But if you prefer I can absolutely replace TPM_DIGEST_SIZE by
hash_digest_size[ima_algo_array[i].algo].

Thanks,

Nicolai

>
>>  			continue;
>>  		}
>>  
>

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index dfdcbd009720..23ded8ea47dc 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -226,16 +226,34 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   * @chip:	TPM chip to use.
>>   * @pcr_idx:	index of the PCR.
>>   * @digests:	list of pcr banks and corresponding digest values to extend.
>> + * @banks_skip_mask:	pcr banks to skip
>>   *
>>   * Return: Same as with tpm_transmit_cmd.
>>   */
>>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>> -		    struct tpm_digest *digests)
>> +		    struct tpm_digest *digests,
>> +		    unsigned long banks_skip_mask)
>>  {
>>  	struct tpm_buf buf;
>> +	unsigned long skip_mask;
>> +	u32 banks_count;
>>  	int rc;
>>  	int i;
>>  
>> +	banks_count = 0;
>> +	skip_mask = banks_skip_mask;
>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>> +		const bool skip_bank = skip_mask & 1;
>> +
>> +		skip_mask >>= 1;
>> +		if (skip_bank)
>> +			continue;
>> +		banks_count++;
>> +	}
>
> Setting ima_unsupported_pcr_banks_mask used BIT(i).  Testing the bit should be
> as straight forward here and below.

I opted for not to using BIT(i) here because in theory
->nr_allocated_banks could be > BITS_PER_LONG. Not in practice though,
but I felt it would improve code readabily if there aren't any implict
assumptions. Also I'm not sure static checkers wouldn't complain about
for (i = 0; i < a; i++) {  1ul << i; }

Anyway, I'm realizing now that the code above is effectively just a
popcnt implementation on the lower bits of ~banks_skip_mask.

IMO it would be perhaps even better to do

	unsigned long skipped_banks_count, banks_count;

	skipped_banks_count = 0;
	skip_mask = banks_skip_mask;
	for (i = 0; skip_mask && i < chip->nr_allocated_banks; i++) {
        	skipped_banks_count += skip_mask & 1;
                skip_mask >>= 1;
	}
        banks_count = chip->nr_allocated_banks - skipped_banks_count;

instead. That way it's almost a nop in the common case of a clear
banks_skip_mask, plus there are no conditionals in the body.


> The first TPM extend after boot is the boot_aggregate.  Afterwards the number of
> banks being extended should always be the same.  Do we really need to re-
> calculate the number of banks needing to be extended each time?
>

Otherwise the number of banks to skip would have to get stored somewhere
and passed around, IIUC. I don't think that's worth it, the total number
of allocated banks is expected to be relatively small and
banks_skip_mask is zero in the common case anyway.

Thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 07/13] tpm: enable bank selection for PCR extend

[Nicolai Stange]

Jarkko Sakkinen <jarkko@kernel.org> writes:

> On Sun, Mar 23, 2025 at 03:09:05PM +0100, Nicolai Stange wrote:
>> The existing tpm_pcr_extend() extends all of a PCR's allocated banks with
>> the corresponding digest from the provided digests[] argument.
>
> Why not "just" tpm_pcr_extend(). We don't have a concept of
> "non-existing tpm_pcr_extend()".
>
> "tpm_pcr_extend() extends the allocated PCR banks ..."
>
> or something.

Right.


>> 
>> An upcoming code change to IMA will introduce the need to skip over those
>
> Don't talk about upcoming code changes. Just explain why IMA depends on
> the change.

Ok.


>> banks it does not have a hash algorithm implementation available for.
>> 
>> Introduce tpm_pcr_extend_sel() to support this.
>> 
>> tpm_pcr_extend_sel() also expects a digests[] array, always being the
>> number of allocated PCR banks in size, just as it's the case for the
>> existing tpm_pcr_extend(). In addition to that however, it takes a
>> 'banks_skip_mask', and will skip the extension of any bank having its
>> corresponding bit set there.
>> 
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>> ---
>>  drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++--
>>  drivers/char/tpm/tpm.h           |  3 ++-
>>  drivers/char/tpm/tpm2-cmd.c      | 29 +++++++++++++++++++++++++++--
>>  include/linux/tpm.h              |  3 +++
>>  4 files changed, 59 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index b1daa0d7b341..88b4496de1df 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -314,6 +314,26 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>   */
>>  int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>>  		   struct tpm_digest *digests)
>> +{
>> +	return tpm_pcr_extend_sel(chip, pcr_idx, digests, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>
> I'd add just an extra argument to tpm_pcr_extend().

Perfect, will do.

Thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 08/13] ima: track the set of PCRs ever extended

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
>> Right now, PCR banks with unsupported hash algorithms are getting
>> invalidated over and over again for each new measurement list entry
>> recorded.
>> 
>> A subsequent patch will make IMA to invalidate PCR banks associated with
>> unsupported hash algorithms only once at a PCR's first use. To prepare for
>> that, make it track the set of PCRs ever extended.
>> 
>> Maintain the set of touched PCRs in an unsigned long bitmask,
>> 'ima_extended_pcrs_mask'.
>> 
>> Amend the IMA_INVALID_PCR() #define to check that a given PCR can get
>> represented in that bitmask. Note that this is only for improving code
>> maintainablity, it does not actually constain the set of allowed PCR
>> indices any further.
>> 
>> Make ima_pcr_extend() to maintain the ima_extended_pcrs_mask, i.e. to set
>> the currently extented PCR's corresponding bit.
>> 
>> Note that at this point there's no provision to restore the
>> ima_extended_pcrs_mask value after kexecs yet, that will be the subject of
>> later patches.
>> 
>> Signed-off-by: Nicolai Stange <nstange@suse.de>
>
> Hi Nicolai,
>
> IMA extends measurements in the default TPM PCR based on the Kconfig
> CONFIG_IMA_MEASURE_PCR_IDX option.  Normally that is set to PCR 10.  The IMA
> policy rules may override the default PCR with a per policy rule
> specific PCR.

Yes, that matches my understanding.


> INVALID_PCR() checks the IMA policy rule specified is a valid PCR register.
>
> Is the purpose of this patch to have a single per TPM bank violation or multiple
> violations, one for each PCR used within the TPM bank?

One for each PCR individually, issued when a given PCR is being
referenced for the first time from some IMA event.

Thanks!

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Mimi Zohar]

On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote:
> Mimi Zohar <zohar@linux.ibm.com> writes:
> 
> > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> > > runtime_measurements_<hash-algo> sysfs files are getting created for
> > > each PCR bank + for SHA-1.
> > > 
> > > Now that runtime_measurements_<hash-algo> sysfs file creation is being
> > > skipped for unsupported hash algorithms, it will become possible that no
> > > such file would be provided at all once SHA-1 is made optional in a
> > > later patch.
> > > 
> > > Always create the file for the 'ima_hash' algorithm, even if it's not
> > > associated with any of the PCR banks. As IMA initialization will
> > > continue to fail if the ima_hash algorithm is not available to the
> > > kernel, this guarantees that at least one such file will always be
> > > there.
> > > 
> > > Signed-off-by: Nicolai Stange <nstange@suse.de>
> > > ---
> > >  security/integrity/ima/ima_fs.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > > index a8df2fe5f4cb..f030ff7f56da 100644
> > > --- a/security/integrity/ima/ima_fs.c
> > > +++ b/security/integrity/ima/ima_fs.c
> > > @@ -436,10 +436,8 @@ static int __init create_securityfs_measurement_lists(void)
> > >  	u16 algo;
> > >  	int i;
> > >  
> > > -	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
> > > -
> > > -	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
> > > -		securityfs_measurement_list_count++;
> > > +	securityfs_measurement_list_count =
> > > +		NR_BANKS(ima_tpm_chip) + ima_extra_slots;
> > >  
> > >  	ascii_securityfs_measurement_lists =
> > >  	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
> > 
> > "ima_hash" is the default file hash algorithm.  Re-using it as the default
> > complete measurement list assumes that the subsequent kexec'ed kernels configure
> > and define it as the default file hash algorithm as well, which might not be the
> > case.
> 
> I don't really see why the ima_hashes would have to match between kexecs
> for this to work -- all events' template hashes are getting recreated
> from scratch anyway after kexec (ima_restore_measurement_list() ->
> ima_calc_field_array_hash()).
> 
> That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one
> would have *runtime_measurements_sha256 first and
> *runtime_measurements_sha384 after kexec. And both had exclusively
> template hashes of their respective algo in them each.
> 
> What am I missing?

Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed
to be built into the kernel.  It's highly unlikely someone would choose a hash
algorithm not built into kernel, but it is possible.  hash_setup() only verifies
that the hash algorithm is a valid name.

Either fix hash_setup() to guarantee that the chosen hash algorithm is built
into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select
the hash algorithm.  This would be in lieu of v2 05/13.

> > Drop this patch.
> 
> Fine by me, but just to confirm, in case there's no TPM attached and
> SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all
> then. Is that Ok?

Of course not.  :)

> ima_hash was chosen here only, because after this series, it will be the
> only single algorithm guaranteed to be available.

With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256
from Kconfig', SHA256 would be added to the "extra" measurements if the TPM
SHA256 bank is disabled.

Mimi

Re: [RFC PATCH v2 01/13] ima: don't expose runtime_measurements for unsupported hashes

[Mimi Zohar]

> > > I would argue that it's likely that no existing userspace tool is relying
> > > on this fallback logic -- they either wouldn't consume the hash value from
> > > the measurement list directly but recreate it by themselves, as is required
> > > for verification against PCRs, or, if they did, they would somehow assume a
> > > hash algorithm and expect the hashes in the measurement list to be of that
> > > type. If of the latter kind, this could even lead to hard to debug
> > > verification failures. For example, from looking at keylime's current
> > > code, the verifier logic seems to assume that the template hashes found
> > > in the provided measurement list are of the configured 'ima_log_hash_alg'
> > > type. In particular, it does not check against padded SHA1 upon
> > > mismatch.
> > 
> > The downside, if none of the TPM bank hash algorithms are configured as builtin
> > in the kernel, is the lack of a measurement list.
> 
> Yes. Just for the record, going forward SHA256 will be, with [v2 05/13]. So
> unless the SHA256 bank is disabled by firmware, it should be fine then.

When IMA goes into TPM-bypass mode, the measurement list still needs to exist. 
We cannot depend on the SHA256 TPM bank being enabled, nor SHA256 always being
the default IMA file hash algorithm.

For this reason sha256 [v2 05/13] needs to be added to the list of "extra"
hashes, if the TPM sha256 bank is disabled.

Refer to the comments on [02/13] for alternatives.

> > If the purpose of this patch set is to actually remove IMA's dependency on a
> > working SHA-1, at some point the Kconfig "select CRYPTO_SHA1" needs to be
> > removed.  Otherwise the kernel will be built with SHA1 builtin
> > (CONFIG_CRYPTO_SHA1=y).
> 
> I should have described it better. In the first step at least, the goal
> is to remove the runtime dependency only. Because when SHA1's
> ->fips_allowed in crypto/testmgr.c gets flipped to false, SHA1
> instantiation would fail with -ENOENT if the kernel was booted with a
> fips=1 on its command line. Users not interested in FIPS, i.e. the vast
> majority, might still want to use SHA1 and there's no real reason not
> to.

Thank you for the explanation.

> 
> But yes, it would definitely make sense to drop the CRYPTO_SHA1 dep, at
> least at some point. Perhaps by simply moving it to the new
> IMA_COMPAT_FALLBACK_TPM_EXTEND. I would personally not do that now
> though, just in case there'll be some unexpected fallout from this
> series.

As long as there is a way of testing the changes, I'm fine with not removing the
select.


Mimi

Re: [RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote:
>> Mimi Zohar <zohar@linux.ibm.com> writes:
>> 
>> > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:

>> > "ima_hash" is the default file hash algorithm.  Re-using it as the default
>> > complete measurement list assumes that the subsequent kexec'ed kernels configure
>> > and define it as the default file hash algorithm as well, which might not be the
>> > case.
>> 
>> I don't really see why the ima_hashes would have to match between kexecs
>> for this to work -- all events' template hashes are getting recreated
>> from scratch anyway after kexec (ima_restore_measurement_list() ->
>> ima_calc_field_array_hash()).
>> 
>> That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one
>> would have *runtime_measurements_sha256 first and
>> *runtime_measurements_sha384 after kexec. And both had exclusively
>> template hashes of their respective algo in them each.
>> 
>> What am I missing?
>
> Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed
> to be built into the kernel.  It's highly unlikely someone would choose a hash
> algorithm not built into kernel, but it is possible.  hash_setup() only verifies
> that the hash algorithm is a valid name.

But ima_init_ima_crypto(), hence the whole IMA __init, would fail if
ima_hash was unavailable at __init time?

Thanks,

Nicolai

> Either fix hash_setup() to guarantee that the chosen hash algorithm is built
> into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select
> the hash algorithm.  This would be in lieu of v2 05/13.
>
>> > Drop this patch.
>> 
>> Fine by me, but just to confirm, in case there's no TPM attached and
>> SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all
>> then. Is that Ok?
>
> Of course not.  :)
>
>> ima_hash was chosen here only, because after this series, it will be the
>> only single algorithm guaranteed to be available.
>
> With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256
> from Kconfig', SHA256 would be added to the "extra" measurements if the TPM
> SHA256 bank is disabled.


-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Mimi Zohar]

On Wed, 2025-03-26 at 10:01 +0100, Nicolai Stange wrote:
> Mimi Zohar <zohar@linux.ibm.com> writes:
> 
> > > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> > > index 6f5696d999d0..a43080fb8edc 100644
> > > --- a/security/integrity/ima/ima_crypto.c
> > > +++ b/security/integrity/ima/ima_crypto.c
> > > @@ -625,26 +625,43 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
> > >  	u16 alg_id;
> > >  	int rc, i;
> > >  
> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
> > >  	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
> > >  	if (rc)
> > >  		return rc;
> > >  
> > >  	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
> > > +#endif
> > >  
> > >  	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
> > >  		if (i == ima_sha1_idx)
> > >  			continue;
> > > +#endif
> > >  
> > >  		if (i < NR_BANKS(ima_tpm_chip)) {
> > >  			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
> > >  			entry->digests[i].alg_id = alg_id;
> > >  		}
> > >  
> > > -		/* for unmapped TPM algorithms digest is still a padded SHA1 */
> > > +		/*
> > > +		 * For unmapped TPM algorithms, the digest is still a
> > > +		 * padded SHA1 if backwards-compatibility fallback PCR
> > > +		 * extension is enabled. Otherwise fill with
> > > +		 * 0xfes. This is the value to invalidate unsupported
> > > +		 * PCR banks with. Also, a non-all-zeroes value serves
> > > +		 * as an indicator to kexec measurement restoration
> > > +		 * that the entry is not a violation and all its
> > > +		 * template digests need to get recomputed.
> > > +		 */
> > >  		if (!ima_algo_array[i].tfm) {
> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
> > >  			memcpy(entry->digests[i].digest,
> > >  			       entry->digests[ima_sha1_idx].digest,
> > >  			       TPM_DIGEST_SIZE);
> 
>                                ^
> That's been here before, just for the record for the below.

And it is correct.

> 
> > > +#else
> > > +			memset(entry->digests[i].digest, 0xfe, TPM_DIGEST_SIZE);
> > > +#endif
> > 
> > Using TPM_DIGEST_SIZE will result in a padded 0xfe value.
> 
> Yes, but as the sysfs files for unsupported algos are gone, this will be
> used only for extending the PCR banks. tpm[12]_pcr_extend()
> (necessarily) truncate the digests to the correct size before sending
> them to the TPM.
> 
> But if you prefer I can absolutely replace TPM_DIGEST_SIZE by
> hash_digest_size[ima_algo_array[i].algo].

Unlike violations, which are the full digest size, a padded sha1 is extended
into the unsupported algos TPM banks.  I assume you'd want it to be the full
digest size like violations.

Mimi

Re: [RFC PATCH v2 03/13] ima: invalidate unsupported PCR banks

[Nicolai Stange]

Mimi Zohar <zohar@linux.ibm.com> writes:

> On Wed, 2025-03-26 at 10:01 +0100, Nicolai Stange wrote:
>> Mimi Zohar <zohar@linux.ibm.com> writes:
>> 
>> > > diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
>> > > index 6f5696d999d0..a43080fb8edc 100644
>> > > --- a/security/integrity/ima/ima_crypto.c
>> > > +++ b/security/integrity/ima/ima_crypto.c
>> > > @@ -625,26 +625,43 @@ int ima_calc_field_array_hash(struct ima_field_data *field_data,
>> > >  	u16 alg_id;
>> > >  	int rc, i;
>> > >  
>> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>> > >  	rc = ima_calc_field_array_hash_tfm(field_data, entry, ima_sha1_idx);
>> > >  	if (rc)
>> > >  		return rc;
>> > >  
>> > >  	entry->digests[ima_sha1_idx].alg_id = TPM_ALG_SHA1;
>> > > +#endif
>> > >  
>> > >  	for (i = 0; i < NR_BANKS(ima_tpm_chip) + ima_extra_slots; i++) {
>> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>> > >  		if (i == ima_sha1_idx)
>> > >  			continue;
>> > > +#endif
>> > >  
>> > >  		if (i < NR_BANKS(ima_tpm_chip)) {
>> > >  			alg_id = ima_tpm_chip->allocated_banks[i].alg_id;
>> > >  			entry->digests[i].alg_id = alg_id;
>> > >  		}
>> > >  
>> > > -		/* for unmapped TPM algorithms digest is still a padded SHA1 */
>> > > +		/*
>> > > +		 * For unmapped TPM algorithms, the digest is still a
>> > > +		 * padded SHA1 if backwards-compatibility fallback PCR
>> > > +		 * extension is enabled. Otherwise fill with
>> > > +		 * 0xfes. This is the value to invalidate unsupported
>> > > +		 * PCR banks with. Also, a non-all-zeroes value serves
>> > > +		 * as an indicator to kexec measurement restoration
>> > > +		 * that the entry is not a violation and all its
>> > > +		 * template digests need to get recomputed.
>> > > +		 */
>> > >  		if (!ima_algo_array[i].tfm) {
>> > > +#if IS_ENABLED(CONFIG_IMA_COMPAT_FALLBACK_TPM_EXTEND)
>> > >  			memcpy(entry->digests[i].digest,
>> > >  			       entry->digests[ima_sha1_idx].digest,
>> > >  			       TPM_DIGEST_SIZE);
>> 
>>                                ^
>> That's been here before, just for the record for the below.
>
> And it is correct.
>
>> 
>> > > +#else
>> > > +			memset(entry->digests[i].digest, 0xfe, TPM_DIGEST_SIZE);
>> > > +#endif
>> > 
>> > Using TPM_DIGEST_SIZE will result in a padded 0xfe value.
>> 
>> Yes, but as the sysfs files for unsupported algos are gone, this will be
>> used only for extending the PCR banks. tpm[12]_pcr_extend()
>> (necessarily) truncate the digests to the correct size before sending
>> them to the TPM.
>> 
>> But if you prefer I can absolutely replace TPM_DIGEST_SIZE by
>> hash_digest_size[ima_algo_array[i].algo].
>
> Unlike violations, which are the full digest size, a padded sha1 is extended
> into the unsupported algos TPM banks.  I assume you'd want it to be the full
> digest size like violations.

You are so right, thanks for spotting! What I wanted is
TPM_MAX_DIGEST_SIZE, not TPM_DIGEST_SIZE (== 20).

Thanks,

Nicolai

-- 
SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany
GF: Ivo Totev, Andrew McDonald, Werner Knoblich
(HRB 36809, AG Nürnberg)

Re: [RFC PATCH v2 02/13] ima: always create runtime_measurements sysfs file for ima_hash

[Mimi Zohar]

On Wed, 2025-03-26 at 14:46 +0100, Nicolai Stange wrote:
> Mimi Zohar <zohar@linux.ibm.com> writes:
> 
> > On Wed, 2025-03-26 at 09:21 +0100, Nicolai Stange wrote:
> > > Mimi Zohar <zohar@linux.ibm.com> writes:
> > > 
> > > > On Sun, 2025-03-23 at 15:09 +0100, Nicolai Stange wrote:
> 
> > > > "ima_hash" is the default file hash algorithm.  Re-using it as the default
> > > > complete measurement list assumes that the subsequent kexec'ed kernels configure
> > > > and define it as the default file hash algorithm as well, which might not be the
> > > > case.
> > > 
> > > I don't really see why the ima_hashes would have to match between kexecs
> > > for this to work -- all events' template hashes are getting recreated
> > > from scratch anyway after kexec (ima_restore_measurement_list() ->
> > > ima_calc_field_array_hash()).
> > > 
> > > That is, if ima_hash=sha256 first, and ima_hash=sha384 after kexec, one
> > > would have *runtime_measurements_sha256 first and
> > > *runtime_measurements_sha384 after kexec. And both had exclusively
> > > template hashes of their respective algo in them each.
> > > 
> > > What am I missing?
> > 
> > Your solution would work nicely, if the "ima_hash" algorithm could be guaranteed
> > to be built into the kernel.  It's highly unlikely someone would choose a hash
> > algorithm not built into kernel, but it is possible.  hash_setup() only verifies
> > that the hash algorithm is a valid name.
> 
> But ima_init_ima_crypto(), hence the whole IMA __init, would fail if
> ima_hash was unavailable at __init time?

Thanks for pointing that out!  Now I understand why just selecting SHA256 is
sufficient.

Mimi

> 
> > Either fix hash_setup() to guarantee that the chosen hash algorithm is built
> > into the kernel or use the CONFIG_IMA_DEFAULT_HASH and add a Kconfig to select
> > the hash algorithm.  This would be in lieu of v2 05/13.
> > 
> > > > Drop this patch.
> > > 
> > > Fine by me, but just to confirm, in case there's no TPM attached and
> > > SHA1 was disabled, there would be no /sys/*/*runtime_measurement* at all
> > > then. Is that Ok?
> > 
> > Of course not.  :)
> > 
> > > ima_hash was chosen here only, because after this series, it will be the
> > > only single algorithm guaranteed to be available.
> > 
> > With the proposed changes to "[RFC PATCH v2 05/13] ima: select CRYPTO_SHA256
> > from Kconfig', SHA256 would be added to the "extra" measurements if the TPM
> > SHA256 bank is disabled.
> 
>