Re: [PATCH 1/3] tpm: Disable TCG_TPM2_HMAC by default

[Jarkko Sakkinen]

On Mon May 27, 2024 at 10:53 PM EEST, Jarkko Sakkinen wrote:
> On Mon May 27, 2024 at 8:57 PM EEST, James Bottomley wrote:
> > On Mon, 2024-05-27 at 18:34 +0300, Jarkko Sakkinen wrote:
> > > On Mon May 27, 2024 at 6:12 PM EEST, Jarkko Sakkinen wrote:
> > > > On Mon May 27, 2024 at 6:01 PM EEST, Jarkko Sakkinen wrote:
> > > > > On Mon May 27, 2024 at 5:51 PM EEST, Jarkko Sakkinen wrote:
> > > > > > On Thu May 23, 2024 at 10:59 AM EEST, Vitor Soares wrote:
> > > > > > > On Wed, 2024-05-22 at 19:11 +0300, Jarkko Sakkinen wrote:
> > > > > > > > On Wed May 22, 2024 at 5:58 PM EEST, Vitor Soares wrote:
> > > > > > > > > I did run with ftrace, but need some more time to go
> > > > > > > > > through it.
> > > > > > > > > 
> > > > > > > > > Here the step I did:
> > > > > > > > > kernel config:
> > > > > > > > >   CONFIG_FUNCTION_TRACER
> > > > > > > > >   CONFIG_FUNCTION_GRAPH_TRACER
> > > > > > > > > 
> > > > > > > > > ftrace:
> > > > > > > > >   # set filters
> > > > > > > > >   echo tpm* > set_ftrace_filter
> > > > > > > > > 
> > > > > > > > >   # set tracer
> > > > > > > > >   echo function_graph > current_tracer
> > > > > > > > > 
> > > > > > > > >   # take the sample
> > > > > > > > >   echo 1 > tracing_on; time modprobe tpm_tis_spi; echo 0
> > > > > > > > > > tracing_on
> > > > > > > > > 
> > > > > > > > > regards,
> > > > > > > > > Vitor Soares
> > > > > > > > 
> > > > > > > > I'm now compiling distro kernel (OpenSUSE) for NUC7 with
> > > > > > > > v6.10 contents.
> > > > > > > > 
> > > > > > > > After I have that setup, I'll develop a perf test either
> > > > > > > > with perf or
> > > > > > > > bpftrace. I'll come back with the possible CONFIG_* that
> > > > > > > > should be in
> > > > > > > > place in your kernel. Might take up until next week as I
> > > > > > > > have some
> > > > > > > > conference stuff to prepare but I try to have stuff ready
> > > > > > > > early next
> > > > > > > > week.
> > > > > > > > 
> > > > > > > > No need to rush with this as long as possible patches go to
> > > > > > > > rc2 or rc3.
> > > > > > > > Let's do a proper analysis instead.
> > > > > > > > 
> > > > > > > > In the meantime you could check if you get perf and/or
> > > > > > > > bpftrace to 
> > > > > > > > your image that use to boot up your device. Preferably both
> > > > > > > > but
> > > > > > > > please inform about this.
> > > > > > > > 
> > > > > > > 
> > > > > > > I already have perf running, for the bpftrace I might not be
> > > > > > > able to help.
> > > > > > 
> > > > > > The interesting function to look at with/without hmac is
> > > > > > probably
> > > > > > tpm2_get_random().
> > > > > > 
> > > > > > I attached a patch that removes hmac shenigans out of
> > > > > > tpm2_get_random()
> > > > > > for the sake of proper comparative testing.
> > > > > 
> > > > > Other thing that we need to measure is to split the cost into
> > > > > two parts:
> > > > > 
> > > > > 1. Handshake, i.e. setting up and shutdowning a session.
> > > > > 2. Transaction, payload TPM command.
> > > > > 
> > > > > This could be done by setting up couple of kprobes_events:
> > > > > 
> > > > >   payload_event: tpm2_get_random() etc.
> > > > >   hmac_event: tpm2_start_auth_session(), tpm2_end_auth_session()
> > > > > etc.
> > > > > 
> > > > > And just summing up the time for a boot to get a cost for hmac.
> > > > > 
> > > > > I'd use bootconfig for this:
> > > > > 
> > > > > https://www.kernel.org/doc/html/v6.9/trace/boottime-trace.html
> > > > > 
> > > > > So I've made up plans how measure the incident but not sure when
> > > > > I
> > > > > have time to pro-actively work on a benchmark (thus sharing
> > > > > details).
> > > > > 
> > > > > So I think with just proper bootconfig wtih no other tools uses
> > > > > this
> > > > > can be measured.
> > > > 
> > > > 
> > > > I'll disable this for anything else than X86_64 by default, and put
> > > > such patch to my next pull request.
> > > > 
> > > > Someone needs to do the perf analysis properly based on the above
> > > > descriptions. I cannot commit my time to promise them to get the
> > > > perf regressions fixed by time. I can only commit on limiting the
> > > > feature ;-)
> > > > 
> > > > It is thus better be conservative and reconsider opt-in post 6.10.
> > > > X86_64 is safeplay because even in that 2018 NUC7 based on Celeron,
> > > > hmac is just fine.
> > > 
> > > While looking at code I started to wanted what was the reasoning
> > > for adding *undocumented* "TPM2_OA_TMPL" in include/linux/tpm.h.
> > > It should really be in tpm2-sessions.c and named something like
> > > TPM2_NULL_KEY_OA or similar.
> >
> > Well, because you asked for it. I originally had all the flags spelled
> > out and I'm not a fan of this obscurity, but you have to do stuff like
> > this to get patches accepted:
> >
> > https://lore.kernel.org/linux-integrity/CZCKTWU6ZCC9.2UTEQPEVICYHL@suppilovahvero/
>
> I still think the constant does make sense.
>
> The current constant does not really imply that it is for the null key,
> it is defined in the wrong file and has no actual legit documentation
> to go with it.

Thus, being conservative and enabling only for x86_64 is pretty balanced
choice for v6.10. The feature really needs to mature before widening the
scope.

The only platform I'm confident is x86_64 because even the old NUC did
good job, so for that part I'm not too worried. Otherwise, it would be
pure guesswork to unconditionally eanble for all arch's.

Those who want to turn the feature  on,still can but we should not make
that decision for them. E.g. perhaps PowerPC could do this but I don't
have any hardware to test it out, and have zero usage reports.

Obviously can be reconsidered to future kernel versions but right now
this feels like the most legit way to act.

BR, Jarkko