Re: [GIT PULL] TPM DEVICE DRIVER: tpmdd-next-v6.18

[Jarkko Sakkinen]

On Mon, Oct 06, 2025 at 05:18:13PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 06, 2025 at 05:13:02PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 06, 2025 at 02:58:17PM +0300, Jarkko Sakkinen wrote:
> > > On Sun, Oct 05, 2025 at 11:09:08AM -0700, Linus Torvalds wrote:
> > > > On Sun, 5 Oct 2025 at 08:47, Jarkko Sakkinen <jarkko@xxxxxxxxxx> wrote:
> > > > >
> > > > >      This pull request disables
> > > > > TCG_TPM2_HMAC from the default configuration as it does not perform well
> > > > > enough [1].
> > > > >
> > > > > [1] https://lore.kernel.org/linux-integrity/20250825203223.629515-1-jarkko@xxxxxxxxxx/
> > > > 
> > > > This link is entirely useless, and doesn't explain what the problem
> > > > was and *why* TPM2_TCG_HMAC shouldn't be on by default.
> > > > 
> > > > I think a much better link is
> > > > 
> > > >    https://lore.kernel.org/linux-integrity/20250814162252.3504279-1-cfenn@xxxxxxxxxx/
> > > > 
> > > > which talks about the problems that TPM2_TCG_HMAC causes.
> > > > 
> > > > Which weren't just about "not performing well enough", but actually
> > > > about how it breaks TPM entirely for some cases.
> > > 
> > > Fair enough. I'll also enumerate the issues, and also roadmap
> > > to heal the feature.
> > 
> > So some of the arguments in Chris' email are debatable, such as
> > this list:
> > 
> > - TPM_RH_NULL
> > - TPM2_CreatePrimary
> > - TPM2_ContextSave
> > - ECDH-P256
> > - AES-128-CFB
> > 
> > 
> > We've never encountered a TPM chip without those TPM commands, and e.g.
> > /dev/tpmrm0 heavily relies on TPM2_ContextSave, and has been in the
> > mainline since 2017. And further, this has been the case on ARM too.
> > 
> > So using all of the arguments as rationale for the change that according
> > to Chris' email are broken because I cannnot objectively on all of the
> > arguments.
> > 
> > E.g. I have to assume to this day that all known TPM chips have those
> > commands because no smoking gun exists. And if DID exist, then I'd
> > assume someone would fixed /dev/tpmrm0 ages ago.
> > 
> > That said, I do agree on disabling the feature for the time being:
> > we have consensus on actions but not really on stimulus tbh.
> > And if there is stimulus I would postpone this patch to create
> > fix also for /dev/tpmrm0.
> > 
> > Argument where I meet with Chris suggestion are:
> > 
> > 1. Performance. The key generation during boot is extremely bad
> >    idea and depending on the deployment the encryption cost is
> >    too much (e.g. with my laptop having fTPM it does not really
> >    matter).
> > 2. Null seed was extremely bad idea. The way I'm planning to actually
> >    fix this is to parametrize the primary key to a persistent key handle
> >    stored into nvram of the chip instead of genration. This will address
> >    also ambiguity and can be linked directly to vendor ceritifcate
> >    for e.g. to perfom remote attesttion.
> > 
> > Things don't go broken by saying that they are broken and nothing
> > elsewhere in the mainline has supporting evidence that those commands
> > would be optional. I cannot agree on argument which I have zero
> > means to measure in any possible way.
> > 
> > This is exactly also the root reason why I wrote my own commit instead
> > with the same change: I could have never signed off the commit that
> > I don't believe is true in its storyline.
> > 
> > So if I write cover for the pull request where I use the subset of
> > arguments with shared consensus would that be enough to get this
> > through? As for primary key handle fix I rather do that with
> > time and proper care.
> 
> I had to use few hours to remind why I did my commit instead of acking
> the original and this is the root. We've never had e.g. a bug in the
> wild that would /dev/tpmrm0 to be broken because ContextSave is not
> available, and it is *widely* used device across all major platforms.

Here's mobile client profile:

https://trustedcomputinggroup.org/wp-content/uploads/TPM_2.0_Mobile_Common_Profile_v2r31_FINAL.pdf

Unless I missed a tidbit I see nothing in it saying that ContextSave
would be optional. If there was even a known legit spec bringing some
context to the claims, that would move things forward.

Section 2.3 states this about ContextSave:

"The symmetric cipher mode TPM_ALG_CFB is REQUIRED by TCG TPM 2.0
Library specification Part 1 [1] and is also necessary for
implementation of TPM2_Create, TPM2_Load, TPM 2_ContextSave,
TPM2_ContextLoad, and other TPM commands"

which actually claims that TPM_ALG_CFB is required where as Chris'
patch claims 180 degrees opposite what the spec says.

Perhaps there's some other random TCG spec that I've missed, it's
entirely possible...

BR, Jarkko