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

[Mimi Zohar <zohar@linux.ibm.com>]

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(-)
>