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

[Jarkko Sakkinen]

On Wed Oct 16, 2024 at 12:29 AM EEST, Jarkko Sakkinen wrote:
> 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.

Back in 2018 at LA, I think it was LSS, there was BoF where this was
discussed I said that for me this feature does not necessarily make
sense since data centers tend to have armed guards, and not black hat
would ever take a even a minor risk of getting hole in the head :-)

After that the whole ecosystem has changed, especially thanks to what
Apple has done with their security chip and user friendly encrypted
boot process, and that has reflected to systemd and the use TPM2,
and thus as a feature bus protection has become relevant.

So also based on these old conclusions I had I fully agree that we
need such a flag to balance things between desktop/laptop and server
use cases, which are both quite relevant. E.g. just me personally
I really enjoy the experience of being able to boot my ThinkPad 
with encryption and without having to type a passphrase per
boot.

I.e. the buy-in part is totally addressed as far as I'm concerned :-)

BR, Jarkko