Re: [RFC PATCH] tpm: Allow the TPM2 pcr_extend HMAC capability to be disabled on boot

[Jarkko Sakkinen]

On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> The initial TPM2 HMAC session capability added HMAC authentication to
> each and every TPM communication making the pcr_extend performance
> abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was
> configured by default on x86_64.
>
> The decision to use the TPM2 HMAC session capability feature doesn't
> differentiate between the critical encrypted and the non-encrypted
> communication, but when configured is required for all TPM communication.
>
> In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit
> 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect
> tpm2_pcr_extend() when used by "trusted keys" to lock the PCR.  However,
> locking the PCR is currently limited to TPM 1.2.
>
> We can revert the commit which adds the HMAC sessions for
> tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be
> disabled on boot for better IMA performance, or define a generic boot
> command line option to disable HMAC in general.  This patch allows
> disabling the HMAC for just the TPM2_pcr_extend.
>
> Fixes: 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()")
> Co-developed-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> ---
> Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch -
> commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open")
>
>  .../admin-guide/kernel-parameters.txt         |  5 ++
>  drivers/char/tpm/tpm2-cmd.c                   | 41 ++++++++++---
>  drivers/char/tpm/tpm2-sessions.c              | 59 +++++++++++--------
>  include/linux/tpm.h                           |  4 ++
>  4 files changed, 77 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..c7811f32ba28 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6727,6 +6727,11 @@
>  	torture.verbose_sleep_duration= [KNL]
>  			Duration of each verbose-printk() sleep in jiffies.
>  
> +	tpm_pcr_extend_hmac_disable [HW,TPM]
> +			Disable TPM2 pcr_extend HMAC for better IMA
> +			performance. By default is set to true (1).
> +			Mainly needed when using a HW TPM2.

Thanks for doing this! I think the code change itself is pretty good but
maybe we should not emphasize HMAC per se (applies to config flag too but
it is what it is now) but instead that they are encrypted and integrity
protected.

I guess all these features intend to protect data from unintended and
physical access, like in common sense terms.

So like for any possible sysadmin and similar I think this would be something
that anyone could grab:

	tpm_disable_protect_pcrs [HW,TPM]
			Do not protect PCR registers from unintended physical
			access, or interposers in the bus by the means of
			having an encrypted and integrity protected session 
			wrapped around TPM2_PCR_Extend command. Consider this
			in a situation where TPM is heavily utilized by
			IMA, thus protection causing a major performance hit,
			and the space where machines are deployed is by other
			means guarded.

Perhaps a bit long but at least it is clear and helps to make the right choice.

BR, Jarkko