On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> On Thu, 2024-11-07 at 00:26 +0200, 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>
> >
> > I have alternative proposal that hit me today.
> >
> > First an observation: I think this issue shows that we also stress
> > beyond limits desktop configurations with encrypted bus, even tho it
> > is
> > not in the same way visible. This affects bunch of things, including
> > e.g. power consumption. Not a lot but best possible situation would
> > be
> > if callers could be served without any additional stress.
> >
> > A second observation is in [1]:
> >
> > "It is recommended that a TPM implement the RNG in a manner that
> > would
> > allow it to return RNG octets such that, as long as the value of
> > bytesRequested is not greater than the maximum digest size, the
> > frequency of bytesRequested being more than the number of octets
> > available is an infrequent occurrence."
> >
> > I think from this we can derive a fair assumption that with any
> > possible
> > TPM2 chip we can pull a 32 byte value within a single transcation
> > (i.e.
> > matching SHA256 digest size).
> >
> > So based on these facts I think this might be a sweet spot in making
> > a
> > compromise between performance and security:
> >
> > 1. Generate a 32 byte seed every N iterations (calls of
> > tpm2_get_random(). Store it to chip->random_seed.
> > 2. In-between iterations use PRNG to generate the values
> > starting form chip->random_seed.
> >
> > I think N could be fairly large without causing any major difference
> > (even when analyzed through numerical error analysis) between calling
> > TPM2_GetRandom for each and every iteration. And this way bus
> > encryption
> > never has to be disabled.
> >
> > I'd see this as win-win approach.
> >
> > PS. I have no idea what kind of PRNG's kernel provides (never used
> > such).
> >
> > [1] 16.1.TPM2_GetRandom
> >
> > https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-3-Commands.pdf
>
> I'm a bit confused here. It's TPM2_PCR_Extend we have the trouble with
> (as Mimi says in her email that you quoted) not TPM2_GetRandom.
>
> The random number generator reseed occurs in a kernel thread that fires
> about once a minute, so it doesn't show up in really any of the boot
> timings. Plus even with sessions added, what there now isn't a
> significant overhead even to the running kernel given it's asynchronous
> and called infrequently.
Ah, right then we need the boot flag, and my earlier comments to the
parameter apply. I've never used IMA so I don't actually even know in
detail how it is using TPM.
Now that I did some seek I mixed this up with the report:
https://chaos.social/@gromit/113345582873908273
Anyway concerning this issue and patch, my earlier comments still apply.
BR, Jarkko
