[PATCH 0/3] KEYS: trusted: bug fixes

[PATCH 0/3] KEYS: trusted: bug fixes

[Jarkko Sakkinen]

Accumulated bug fixes for trusted keys.

Jarkko Sakkinen (3):
  tpm: Disable TCG_TPM2_HMAC by default
  KEYS: trusted: Fix memory leak in tpm2_key_encode()
  KEYS: trusted: Do not use WARN when encode fails

 drivers/char/tpm/Kconfig                  |  2 +-
 security/keys/trusted-keys/trusted_tpm2.c | 25 +++++++++++++++++------
 2 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.45.1

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

[Jarkko Sakkinen]

Causes performance drop in initialization so needs to be opt-in.
Distributors are capable of opt-in enabling this. Could be also handled by
kernel-command line in the future.

Reported-by: Vitor Soares <ivitro@gmail.com>
Closes: https://lore.kernel.org/linux-integrity/bf67346ef623ff3c452c4f968b7d900911e250c3.camel@gmail.com/#t
Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index e63a6a17793c..db41301e63f2 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -29,7 +29,7 @@ if TCG_TPM
 
 config TCG_TPM2_HMAC
 	bool "Use HMAC and encrypted transactions on the TPM bus"
-	default y
+	default n
 	select CRYPTO_ECDH
 	select CRYPTO_LIB_AESCFB
 	select CRYPTO_LIB_SHA256
-- 
2.45.1

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

[Vitor Soares]

Hi Jarkko,

On Mon, 2024-05-20 at 02:51 +0300, Jarkko Sakkinen wrote:
> Causes performance drop in initialization so needs to be opt-in.
> Distributors are capable of opt-in enabling this. Could be also handled by
> kernel-command line in the future.
> 
> Reported-by: Vitor Soares <ivitro@gmail.com>
> Closes:
> https://lore.kernel.org/linux-integrity/bf67346ef623ff3c452c4f968b7d900911e250c3.camel@gmail.com/#t
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  drivers/char/tpm/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index e63a6a17793c..db41301e63f2 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -29,7 +29,7 @@ if TCG_TPM
>  
>  config TCG_TPM2_HMAC
>         bool "Use HMAC and encrypted transactions on the TPM bus"
> -       default y
> +       default n
>         select CRYPTO_ECDH
>         select CRYPTO_LIB_AESCFB
>         select CRYPTO_LIB_SHA256

I did the test on my side, and with TCG_TPM2_HMAC set to "n" the time to probe
tpm_tis_spi driver has reduced to:
real    0m2.009s
user    0m0.001s
sys     0m0.019s

Thanks for your help.

Best regards,
Vitor Soares

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

[Jarkko Sakkinen]

On Tue May 21, 2024 at 10:03 AM EEST, Vitor Soares wrote:
> Hi Jarkko,
>
> On Mon, 2024-05-20 at 02:51 +0300, Jarkko Sakkinen wrote:
> > Causes performance drop in initialization so needs to be opt-in.
> > Distributors are capable of opt-in enabling this. Could be also handled by
> > kernel-command line in the future.
> > 
> > Reported-by: Vitor Soares <ivitro@gmail.com>
> > Closes:
> > https://lore.kernel.org/linux-integrity/bf67346ef623ff3c452c4f968b7d900911e250c3.camel@gmail.com/#t
> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >  drivers/char/tpm/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index e63a6a17793c..db41301e63f2 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -29,7 +29,7 @@ if TCG_TPM
> >  
> >  config TCG_TPM2_HMAC
> >         bool "Use HMAC and encrypted transactions on the TPM bus"
> > -       default y
> > +       default n
> >         select CRYPTO_ECDH
> >         select CRYPTO_LIB_AESCFB
> >         select CRYPTO_LIB_SHA256
>
> I did the test on my side, and with TCG_TPM2_HMAC set to "n" the time to probe
> tpm_tis_spi driver has reduced to:
> real    0m2.009s
> user    0m0.001s
> sys     0m0.019s
>
> Thanks for your help.
>
> Best regards,
> Vitor Soares

Yeah, well overall benefits still weight a lot. Primary keys are
obviously essential for any use of TPM, so better idea might then
just disable the whole TPM if this does not scale.

But as James pointed out in some other response it is not objectively
clear where performance hit is. I guess it would make sense to analyze
how much hmac vs w/o hmac in the pipe costs for TPM commands.

This benchmark could be done in user space using /dev/tpm0.

Anyway, I did not include this to my PR, which I already sent to Linus.
If anyone wants to make kernel command-line option for hmac, I'm willing
to review that (no bandwidth to do it myself).

BR, Jarkko

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

[James Bottomley]

On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> This benchmark could be done in user space using /dev/tpm0.

Let's actually try that.  If you have the ibmtss installed, the command
to time primary key generation from userspace on your tpm is

time tsscreateprimary -hi n -ecc nistp256


And just for chuckles and grins, try it in the owner hierarchy as well
(sometimes slow TPMs cache this)

time tsscreateprimary -hi o -ecc nistp256

And if you have tpm2 tools, the above commands should be:

time tpm2_createprimary -C n -G ecc256
time tpm2_createprimary -C o -G ecc256

James

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

[Jarkko Sakkinen]

On Tue May 21, 2024 at 3:33 PM EEST, James Bottomley wrote:
> On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > This benchmark could be done in user space using /dev/tpm0.
>
> Let's actually try that.  If you have the ibmtss installed, the command
> to time primary key generation from userspace on your tpm is
>
> time tsscreateprimary -hi n -ecc nistp256
>
>
> And just for chuckles and grins, try it in the owner hierarchy as well
> (sometimes slow TPMs cache this)
>
> time tsscreateprimary -hi o -ecc nistp256
>
> And if you have tpm2 tools, the above commands should be:
>
> time tpm2_createprimary -C n -G ecc256
> time tpm2_createprimary -C o -G ecc256

Thanks, I definitely want to try these in my NUC7. I can try both
stacks and it is pretty good test machine because it is old'ish
and slow ;-)

I'm also thinking differently than when I put out this pull request.
I honestly think that it must be weird use case to use TPM with
a machine that dies with a HMAC pipe. It makes no sense to me and
I think we should focus on common sense here.

I could imagine one use case: pre-production hardware that is not
yet in ASIC. But in that case you would probably build your kernel
picking exactly the right options. I mean it is only a default
after all.

I think we could add this:

	default X86 || ARM64

This pretty covers the spectrum where HMAC does make sense by
default. We can always relax it but this does not really take
away the legit user base from the feature.

It would be a huge bottleneck to make HMAC also opt-in because
the stuff it adds makes a lot of sense when build on top. E.g.
the asymmetric key patch set that I sent within early week was
made possible by all this great work that you've done.

So yeah, I'd like to send the above Kconfig changes, but that
is all I want to do this at this point.

> James

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue May 21, 2024 at 4:00 PM EEST, Jarkko Sakkinen wrote:
> On Tue May 21, 2024 at 3:33 PM EEST, James Bottomley wrote:
> > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > This benchmark could be done in user space using /dev/tpm0.
> >
> > Let's actually try that.  If you have the ibmtss installed, the command
> > to time primary key generation from userspace on your tpm is
> >
> > time tsscreateprimary -hi n -ecc nistp256
> >
> >
> > And just for chuckles and grins, try it in the owner hierarchy as well
> > (sometimes slow TPMs cache this)
> >
> > time tsscreateprimary -hi o -ecc nistp256
> >
> > And if you have tpm2 tools, the above commands should be:
> >
> > time tpm2_createprimary -C n -G ecc256
> > time tpm2_createprimary -C o -G ecc256
>
> Thanks, I definitely want to try these in my NUC7. I can try both
> stacks and it is pretty good test machine because it is old'ish
> and slow ;-)
>
> I'm also thinking differently than when I put out this pull request.
> I honestly think that it must be weird use case to use TPM with
> a machine that dies with a HMAC pipe. It makes no sense to me and
> I think we should focus on common sense here.
>
> I could imagine one use case: pre-production hardware that is not
> yet in ASIC. But in that case you would probably build your kernel
> picking exactly the right options. I mean it is only a default
> after all.
>
> I think we could add this:
>
> 	default X86 || ARM64
>
> This pretty covers the spectrum where HMAC does make sense by
> default. We can always relax it but this does not really take
> away the legit user base from the feature.
>
> It would be a huge bottleneck to make HMAC also opt-in because
> the stuff it adds makes a lot of sense when build on top. E.g.
> the asymmetric key patch set that I sent within early week was
> made possible by all this great work that you've done.
>
> So yeah, I'd like to send the above Kconfig changes, but that
> is all I want to do this at this point.

Patch is out (lore link was not yet available):

https://lkml.org/lkml/2024/5/21/583

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue May 21, 2024 at 4:11 PM EEST, Jarkko Sakkinen wrote:
> On Tue May 21, 2024 at 4:00 PM EEST, Jarkko Sakkinen wrote:
> > On Tue May 21, 2024 at 3:33 PM EEST, James Bottomley wrote:
> > > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > > This benchmark could be done in user space using /dev/tpm0.
> > >
> > > Let's actually try that.  If you have the ibmtss installed, the command
> > > to time primary key generation from userspace on your tpm is
> > >
> > > time tsscreateprimary -hi n -ecc nistp256
> > >
> > >
> > > And just for chuckles and grins, try it in the owner hierarchy as well
> > > (sometimes slow TPMs cache this)
> > >
> > > time tsscreateprimary -hi o -ecc nistp256
> > >
> > > And if you have tpm2 tools, the above commands should be:
> > >
> > > time tpm2_createprimary -C n -G ecc256
> > > time tpm2_createprimary -C o -G ecc256
> >
> > Thanks, I definitely want to try these in my NUC7. I can try both
> > stacks and it is pretty good test machine because it is old'ish
> > and slow ;-)
> >
> > I'm also thinking differently than when I put out this pull request.
> > I honestly think that it must be weird use case to use TPM with
> > a machine that dies with a HMAC pipe. It makes no sense to me and
> > I think we should focus on common sense here.
> >
> > I could imagine one use case: pre-production hardware that is not
> > yet in ASIC. But in that case you would probably build your kernel
> > picking exactly the right options. I mean it is only a default
> > after all.
> >
> > I think we could add this:
> >
> > 	default X86 || ARM64
> >
> > This pretty covers the spectrum where HMAC does make sense by
> > default. We can always relax it but this does not really take
> > away the legit user base from the feature.
> >
> > It would be a huge bottleneck to make HMAC also opt-in because
> > the stuff it adds makes a lot of sense when build on top. E.g.
> > the asymmetric key patch set that I sent within early week was
> > made possible by all this great work that you've done.
> >
> > So yeah, I'd like to send the above Kconfig changes, but that
> > is all I want to do this at this point.
>
> Patch is out (lore link was not yet available):
>
> https://lkml.org/lkml/2024/5/21/583

Right also: TCG_TPM is neither default in x86 defconfig. So it would
require two switches turned on to get basic TPM support ongoing. So
yeah, I think we're in a sweet spot with above patch.

BR, Jarkko

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

[Vitor Soares]

On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > This benchmark could be done in user space using /dev/tpm0.
> 
> Let's actually try that.  If you have the ibmtss installed, the command
> to time primary key generation from userspace on your tpm is
> 
> time tsscreateprimary -hi n -ecc nistp256
> 
> 
> And just for chuckles and grins, try it in the owner hierarchy as well
> (sometimes slow TPMs cache this)
> 
> time tsscreateprimary -hi o -ecc nistp256
> 
> And if you have tpm2 tools, the above commands should be:
> 
> time tpm2_createprimary -C n -G ecc256
> time tpm2_createprimary -C o -G ecc256
> 
> James
> 
> 

Testing on an arm64 platform I get the following results.

hmac disabled:
  time modprobe tpm_tis_spi
  real    0m2.776s
  user    0m0.006s
  sys     0m0.015s

  time tpm2_createprimary -C n -G ecc256
  real    0m0.686s
  user    0m0.044s
  sys     0m0.025s

  time tpm2_createprimary -C o -G ecc256
  real    0m0.638s
  user    0m0.048s
  sys     0m0.009s


hmac enabled:
  time modprobe tpm_tis_spi
  real    8m5.840s
  user    0m0.005s
  sys     0m0.018s


  time tpm2_createprimary -C n -G ecc256
  real    5m27.678s
  user    0m0.059s
  sys     0m0.009s

  (after first command)
  real    0m0.395s
  user    0m0.040s
  sys     0m0.015s

  time tpm2_createprimary -C o -G ecc256
  real    0m0.418s
  user    0m0.049s
  sys     0m0.009s

hmac enabled + patches applied
  time modprobe tpm_tis_spi
  real    8m6.663s
  user    0m0.000s
  sys     0m0.021s


  time tpm2_createprimary -C n -G ecc256
  real    7m24.662s
  user    0m0.048s
  sys     0m0.022s

  (after first command)
  real    0m0.395s
  user    0m0.047s
  sys     0m0.009s

  time tpm2_createprimary -C o -G ecc256
  real    0m0.404s
  user    0m0.046s
  sys     0m0.012s


Regards,
Vitor Soares

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

[Jarkko Sakkinen]

On Wed May 22, 2024 at 11:18 AM EEST, Vitor Soares wrote:
> On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > This benchmark could be done in user space using /dev/tpm0.
> > 
> > Let's actually try that.  If you have the ibmtss installed, the command
> > to time primary key generation from userspace on your tpm is
> > 
> > time tsscreateprimary -hi n -ecc nistp256
> > 
> > 
> > And just for chuckles and grins, try it in the owner hierarchy as well
> > (sometimes slow TPMs cache this)
> > 
> > time tsscreateprimary -hi o -ecc nistp256
> > 
> > And if you have tpm2 tools, the above commands should be:
> > 
> > time tpm2_createprimary -C n -G ecc256
> > time tpm2_createprimary -C o -G ecc256
> > 
> > James
> > 
> > 
>
> Testing on an arm64 platform I get the following results.

OK, appreciate these results. I try to get mine this week, if I can
allocate some bandwidth but latest early next week. The Intel CPU
I'll be testing is Intel Celeron J4025:

https://www.intel.com/content/www/us/en/products/sku/197307/intel-celeron-processor-j4025-4m-cache-up-to-2-90-ghz/specifications.html

So if things work reasonably fast with this, then I think we can
enable the feature at least on X86_64 by default, and make it
opt-in for other arch's.

I sent already this patch but holding with PR up until rc1 is
out so that there is some window to act:

https://lore.kernel.org/linux-integrity/20240521130921.15028-1-jarkko@kernel.org/

If I need to send an updated patch ("default X86_64") and rip
transcrip from below results.

But to do that correctly I'd need to know at least:

1. What is the aarch64 platform you are using?
2. What kind of TPM you are using and how is it connect?

Obviously if I make this decision, I'll put you as "Reported-by".

>
> hmac disabled:
>   time modprobe tpm_tis_spi
>   real    0m2.776s
>   user    0m0.006s
>   sys     0m0.015s
>
>   time tpm2_createprimary -C n -G ecc256
>   real    0m0.686s
>   user    0m0.044s
>   sys     0m0.025s
>
>   time tpm2_createprimary -C o -G ecc256
>   real    0m0.638s
>   user    0m0.048s
>   sys     0m0.009s
>
>
> hmac enabled:
>   time modprobe tpm_tis_spi
>   real    8m5.840s
>   user    0m0.005s
>   sys     0m0.018s
>
>
>   time tpm2_createprimary -C n -G ecc256
>   real    5m27.678s
>   user    0m0.059s
>   sys     0m0.009s
>
>   (after first command)
>   real    0m0.395s
>   user    0m0.040s
>   sys     0m0.015s
>
>   time tpm2_createprimary -C o -G ecc256
>   real    0m0.418s
>   user    0m0.049s
>   sys     0m0.009s
>
> hmac enabled + patches applied
>   time modprobe tpm_tis_spi
>   real    8m6.663s
>   user    0m0.000s
>   sys     0m0.021s
>
>
>   time tpm2_createprimary -C n -G ecc256
>   real    7m24.662s
>   user    0m0.048s
>   sys     0m0.022s
>
>   (after first command)
>   real    0m0.395s
>   user    0m0.047s
>   sys     0m0.009s
>
>   time tpm2_createprimary -C o -G ecc256
>   real    0m0.404s
>   user    0m0.046s
>   sys     0m0.012s
>
>
> Regards,
> Vitor Soares

BR, Jarkko

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

[Vitor Soares]

On Wed, 2024-05-22 at 15:01 +0300, Jarkko Sakkinen wrote:
> On Wed May 22, 2024 at 11:18 AM EEST, Vitor Soares wrote:
> > On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > > This benchmark could be done in user space using /dev/tpm0.
> > > 
> > > Let's actually try that.  If you have the ibmtss installed, the command
> > > to time primary key generation from userspace on your tpm is
> > > 
> > > time tsscreateprimary -hi n -ecc nistp256
> > > 
> > > 
> > > And just for chuckles and grins, try it in the owner hierarchy as well
> > > (sometimes slow TPMs cache this)
> > > 
> > > time tsscreateprimary -hi o -ecc nistp256
> > > 
> > > And if you have tpm2 tools, the above commands should be:
> > > 
> > > time tpm2_createprimary -C n -G ecc256
> > > time tpm2_createprimary -C o -G ecc256
> > > 
> > > James
> > > 
> > > 
> > 
> > Testing on an arm64 platform I get the following results.
> 
> OK, appreciate these results. I try to get mine this week, if I can
> allocate some bandwidth but latest early next week. The Intel CPU
> I'll be testing is Intel Celeron J4025:
> 
> https://www.intel.com/content/www/us/en/products/sku/197307/intel-celeron-processor-j4025-4m-cache-up-to-2-90-ghz/specifications.html
> 
> So if things work reasonably fast with this, then I think we can
> enable the feature at least on X86_64 by default, and make it
> opt-in for other arch's.
> 
> I sent already this patch but holding with PR up until rc1 is
> out so that there is some window to act:
> 
> https://lore.kernel.org/linux-integrity/20240521130921.15028-1-jarkko@kernel.org/
> 
> If I need to send an updated patch ("default X86_64") and rip
> transcrip from below results.
> 
> But to do that correctly I'd need to know at least:
> 
> 1. What is the aarch64 platform you are using?

I was testing this on the Toradex Verdin iMX8MM SoM.

> 2. What kind of TPM you are using and how is it connect?

TPM device is the ATTPM20P connect through the SPI at speed of 36 MHz.
The bus is shared with a CAN controller (MCP251xFD), so both mues work together.

The dts looks like:
tpm1: tpm@1 {
        compatible = "atmel,attpm20p", "tcg,tpm_tis-spi";
        interrupts-extended = <&gpio1 7 IRQ_TYPE_LEVEL_LOW>;
        pinctrl-0 = <&pinctrl_can2_int>;
        pinctrl-names = "default";
        reg = <1>;
        spi-max-frequency = <36000000>;
};

Regards,
Vitor Soares

> 
> Obviously if I make this decision, I'll put you as "Reported-by".
> 
> 

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

[Vitor Soares]

On Wed, 2024-05-22 at 14:17 +0100, Vitor Soares wrote:
> On Wed, 2024-05-22 at 15:01 +0300, Jarkko Sakkinen wrote:
> > On Wed May 22, 2024 at 11:18 AM EEST, Vitor Soares wrote:
> > > On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > > > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > > > This benchmark could be done in user space using /dev/tpm0.
> > > > 
> > > > Let's actually try that.  If you have the ibmtss installed, the command
> > > > to time primary key generation from userspace on your tpm is
> > > > 
> > > > time tsscreateprimary -hi n -ecc nistp256
> > > > 
> > > > 
> > > > And just for chuckles and grins, try it in the owner hierarchy as well
> > > > (sometimes slow TPMs cache this)
> > > > 
> > > > time tsscreateprimary -hi o -ecc nistp256
> > > > 
> > > > And if you have tpm2 tools, the above commands should be:
> > > > 
> > > > time tpm2_createprimary -C n -G ecc256
> > > > time tpm2_createprimary -C o -G ecc256
> > > > 
> > > > James
> > > > 
> > > > 
> > > 
> > > Testing on an arm64 platform I get the following results.
> > 
> > OK, appreciate these results. I try to get mine this week, if I can
> > allocate some bandwidth but latest early next week. The Intel CPU
> > I'll be testing is Intel Celeron J4025:
> > 
> > https://www.intel.com/content/www/us/en/products/sku/197307/intel-celeron-processor-j4025-4m-cache-up-to-2-90-ghz/specifications.html
> > 
> > So if things work reasonably fast with this, then I think we can
> > enable the feature at least on X86_64 by default, and make it
> > opt-in for other arch's.
> > 
> > I sent already this patch but holding with PR up until rc1 is
> > out so that there is some window to act:
> > 
> > https://lore.kernel.org/linux-integrity/20240521130921.15028-1-jarkko@kernel.org/
> > 
> > If I need to send an updated patch ("default X86_64") and rip
> > transcrip from below results.
> > 
> > But to do that correctly I'd need to know at least:
> > 
> > 1. What is the aarch64 platform you are using?
> 
> I was testing this on the Toradex Verdin iMX8MM SoM.
> 
> > 2. What kind of TPM you are using and how is it connect?
> 
> TPM device is the ATTPM20P connect through the SPI at speed of 36 MHz.
> The bus is shared with a CAN controller (MCP251xFD), so both mues work
> together.
> 
> The dts looks like:
> tpm1: tpm@1 {
>         compatible = "atmel,attpm20p", "tcg,tpm_tis-spi";
>         interrupts-extended = <&gpio1 7 IRQ_TYPE_LEVEL_LOW>;
>         pinctrl-0 = <&pinctrl_can2_int>;
>         pinctrl-names = "default";
>         reg = <1>;
>         spi-max-frequency = <36000000>;
> };
> 
> Regards,
> Vitor Soares

For the sake of clarity, the timing tests were done without CAN enabled.

> 
> > 
> > Obviously if I make this decision, I'll put you as "Reported-by".
> > 
> > 
> 

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

[Jarkko Sakkinen]

On Wed May 22, 2024 at 4:17 PM EEST, Vitor Soares wrote:
> > 1. What is the aarch64 platform you are using?
>
> I was testing this on the Toradex Verdin iMX8MM SoM.
>
> > 2. What kind of TPM you are using and how is it connect?
>
> TPM device is the ATTPM20P connect through the SPI at speed of 36 MHz.
> The bus is shared with a CAN controller (MCP251xFD), so both mues work together.
>
> The dts looks like:
> tpm1: tpm@1 {
>         compatible = "atmel,attpm20p", "tcg,tpm_tis-spi";
>         interrupts-extended = <&gpio1 7 IRQ_TYPE_LEVEL_LOW>;
>         pinctrl-0 = <&pinctrl_can2_int>;
>         pinctrl-names = "default";
>         reg = <1>;
>         spi-max-frequency = <36000000>;
> };

Thank you, this exactly what I was looking for. Don't expect any
improvement to the situation before rc1 is out. It is better to
investigate the situation a bit first.

E.g. some people test with fTPM TEE so this was pretty essential
to know that it is a chip going through.

For tpm_crb we should actually disable HMAC at some point. It is
essentially a performance regression for it.

BR, Jarkko

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

[James Bottomley]

On Wed, 2024-05-22 at 17:11 +0300, Jarkko Sakkinen wrote:
> For tpm_crb we should actually disable HMAC at some point. It is
> essentially a performance regression for it.

You'd think that, because of the shared buffer and no bus, but you
never quite know.  For instance several confidential computing early
implementations used the crb interface set up by qemu (i.e. over shared
buffers which are readable by the host).  For them the only way to get
security is with sessions.  Even with the default Intel CRB, the TPM
transaction isn't handled directly by the main CPU, it's offloaded to
the ME (which we all know google loves because of its tight security
boundary).  It is entirely possible to imagine a software interposer
running in the ME snooping the CRB buffer.  A very different type of
attack from the LPB interposer, but plausible non the less.

James

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

[Jarkko Sakkinen]

On Wed May 22, 2024 at 5:20 PM EEST, James Bottomley wrote:
> On Wed, 2024-05-22 at 17:11 +0300, Jarkko Sakkinen wrote:
> > For tpm_crb we should actually disable HMAC at some point. It is
> > essentially a performance regression for it.
>
> You'd think that, because of the shared buffer and no bus, but you
> never quite know.  For instance several confidential computing early
> implementations used the crb interface set up by qemu (i.e. over shared
> buffers which are readable by the host).  For them the only way to get
> security is with sessions.  Even with the default Intel CRB, the TPM
> transaction isn't handled directly by the main CPU, it's offloaded to
> the ME (which we all know google loves because of its tight security
> boundary).  It is entirely possible to imagine a software interposer
> running in the ME snooping the CRB buffer.  A very different type of
> attack from the LPB interposer, but plausible non the less.
>
> James

Should have put "consider". I've tested with crb and spi and have
not noticed anything get stuck. One more reason to run tests with
that Celeron CPU from 2018...

BR, Jarkko

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

[James Bottomley]

On Wed, 2024-05-22 at 09:18 +0100, Vitor Soares wrote:
> On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > This benchmark could be done in user space using /dev/tpm0.
> > 
> > Let's actually try that.  If you have the ibmtss installed, the
> > command to time primary key generation from userspace on your tpm
> > is
> > 
> > time tsscreateprimary -hi n -ecc nistp256
> > 
> > 
> > And just for chuckles and grins, try it in the owner hierarchy as
> > well (sometimes slow TPMs cache this)
> > 
> > time tsscreateprimary -hi o -ecc nistp256
> > 
> > And if you have tpm2 tools, the above commands should be:
> > 
> > time tpm2_createprimary -C n -G ecc256
> > time tpm2_createprimary -C o -G ecc256
> > 
> > James
> > 
> > 
> 
> Testing on an arm64 platform I get the following results.
> 
> hmac disabled:
>   time modprobe tpm_tis_spi
>   real    0m2.776s
>   user    0m0.006s
>   sys     0m0.015s
> 
>   time tpm2_createprimary -C n -G ecc256
>   real    0m0.686s
>   user    0m0.044s
>   sys     0m0.025s
> 
>   time tpm2_createprimary -C o -G ecc256
>   real    0m0.638s
>   user    0m0.048s
>   sys     0m0.009s
> 
> 
> hmac enabled:
>   time modprobe tpm_tis_spi
>   real    8m5.840s
>   user    0m0.005s
>   sys     0m0.018s
> 
> 
>   time tpm2_createprimary -C n -G ecc256
>   real    5m27.678s
>   user    0m0.059s
>   sys     0m0.009s
> 
>   (after first command)
>   real    0m0.395s
>   user    0m0.040s
>   sys     0m0.015s
> 
>   time tpm2_createprimary -C o -G ecc256
>   real    0m0.418s
>   user    0m0.049s
>   sys     0m0.009s

That's interesting: it suggests the create primary is fast (as
expected) but that the TPM is blocked for some reason.  Is there
anything else in dmesg if you do

dmesg|grep -i tpm

?

Unfortunately we don't really do timeouts on our end (we have the TPM
do it instead), but we could instrument your kernel with command and
time sent and returned.  That may tell us where the problem lies.

Regards,

James

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

[Jarkko Sakkinen]

On Wed May 22, 2024 at 4:35 PM EEST, James Bottomley wrote:
> On Wed, 2024-05-22 at 09:18 +0100, Vitor Soares wrote:
> > On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > > This benchmark could be done in user space using /dev/tpm0.
> > > 
> > > Let's actually try that.  If you have the ibmtss installed, the
> > > command to time primary key generation from userspace on your tpm
> > > is
> > > 
> > > time tsscreateprimary -hi n -ecc nistp256
> > > 
> > > 
> > > And just for chuckles and grins, try it in the owner hierarchy as
> > > well (sometimes slow TPMs cache this)
> > > 
> > > time tsscreateprimary -hi o -ecc nistp256
> > > 
> > > And if you have tpm2 tools, the above commands should be:
> > > 
> > > time tpm2_createprimary -C n -G ecc256
> > > time tpm2_createprimary -C o -G ecc256
> > > 
> > > James
> > > 
> > > 
> > 
> > Testing on an arm64 platform I get the following results.
> > 
> > hmac disabled:
> >   time modprobe tpm_tis_spi
> >   real    0m2.776s
> >   user    0m0.006s
> >   sys     0m0.015s
> > 
> >   time tpm2_createprimary -C n -G ecc256
> >   real    0m0.686s
> >   user    0m0.044s
> >   sys     0m0.025s
> > 
> >   time tpm2_createprimary -C o -G ecc256
> >   real    0m0.638s
> >   user    0m0.048s
> >   sys     0m0.009s
> > 
> > 
> > hmac enabled:
> >   time modprobe tpm_tis_spi
> >   real    8m5.840s
> >   user    0m0.005s
> >   sys     0m0.018s
> > 
> > 
> >   time tpm2_createprimary -C n -G ecc256
> >   real    5m27.678s
> >   user    0m0.059s
> >   sys     0m0.009s
> > 
> >   (after first command)
> >   real    0m0.395s
> >   user    0m0.040s
> >   sys     0m0.015s
> > 
> >   time tpm2_createprimary -C o -G ecc256
> >   real    0m0.418s
> >   user    0m0.049s
> >   sys     0m0.009s
>
> That's interesting: it suggests the create primary is fast (as
> expected) but that the TPM is blocked for some reason.  Is there
> anything else in dmesg if you do
>
> dmesg|grep -i tpm
>
> ?
>
> Unfortunately we don't really do timeouts on our end (we have the TPM
> do it instead), but we could instrument your kernel with command and
> time sent and returned.  That may tell us where the problem lies.

If there was possibility to use bpftrace it is trivial to get histogram
of time used where. I can bake a script but I need to know first if it
is available in the first place before going through that trouble.

BR, Jarkko

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

[Vitor Soares]

On Wed, 2024-05-22 at 17:13 +0300, Jarkko Sakkinen wrote:
> On Wed May 22, 2024 at 4:35 PM EEST, James Bottomley wrote:
> > On Wed, 2024-05-22 at 09:18 +0100, Vitor Soares wrote:
> > > On Tue, 2024-05-21 at 08:33 -0400, James Bottomley wrote:
> > > > On Tue, 2024-05-21 at 10:10 +0300, Jarkko Sakkinen wrote:
> > > > > This benchmark could be done in user space using /dev/tpm0.
> > > > 
> > > > Let's actually try that.  If you have the ibmtss installed, the
> > > > command to time primary key generation from userspace on your tpm
> > > > is
> > > > 
> > > > time tsscreateprimary -hi n -ecc nistp256
> > > > 
> > > > 
> > > > And just for chuckles and grins, try it in the owner hierarchy as
> > > > well (sometimes slow TPMs cache this)
> > > > 
> > > > time tsscreateprimary -hi o -ecc nistp256
> > > > 
> > > > And if you have tpm2 tools, the above commands should be:
> > > > 
> > > > time tpm2_createprimary -C n -G ecc256
> > > > time tpm2_createprimary -C o -G ecc256
> > > > 
> > > > James
> > > > 
> > > > 
> > > 
> > > Testing on an arm64 platform I get the following results.
> > > 
> > > hmac disabled:
> > >   time modprobe tpm_tis_spi
> > >   real    0m2.776s
> > >   user    0m0.006s
> > >   sys     0m0.015s
> > > 
> > >   time tpm2_createprimary -C n -G ecc256
> > >   real    0m0.686s
> > >   user    0m0.044s
> > >   sys     0m0.025s
> > > 
> > >   time tpm2_createprimary -C o -G ecc256
> > >   real    0m0.638s
> > >   user    0m0.048s
> > >   sys     0m0.009s
> > > 
> > > 
> > > hmac enabled:
> > >   time modprobe tpm_tis_spi
> > >   real    8m5.840s
> > >   user    0m0.005s
> > >   sys     0m0.018s
> > > 
> > > 
> > >   time tpm2_createprimary -C n -G ecc256
> > >   real    5m27.678s
> > >   user    0m0.059s
> > >   sys     0m0.009s
> > > 
> > >   (after first command)
> > >   real    0m0.395s
> > >   user    0m0.040s
> > >   sys     0m0.015s
> > > 
> > >   time tpm2_createprimary -C o -G ecc256
> > >   real    0m0.418s
> > >   user    0m0.049s
> > >   sys     0m0.009s
> > 
> > That's interesting: it suggests the create primary is fast (as
> > expected) but that the TPM is blocked for some reason.  Is there
> > anything else in dmesg if you do
> > 
> > dmesg|grep -i tpm
> > 
> > ?
> > 
> > Unfortunately we don't really do timeouts on our end (we have the TPM
> > do it instead), but we could instrument your kernel with command and
> > time sent and returned.  That may tell us where the problem lies.
> 
> If there was possibility to use bpftrace it is trivial to get histogram
> of time used where. I can bake a script but I need to know first if it
> is available in the first place before going through that trouble.
> 
> BR, Jarkko

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

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

[Jarkko Sakkinen]

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.

Fair enough?

BR, Jarkko

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

[Vitor Soares]

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.

Regards,
Vitor Soares

> Fair enough?
> 
> BR, Jarkko

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

[Jarkko Sakkinen]

[-- Attachment #1.1: Type: text/plain, Size: 1717 bytes --]

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.

BR, Jarkko

[-- Attachment #2: 0001-fixup.patch --]
[-- Type: text/x-patch, Size: 1663 bytes --]

From 91951ad86ed624416a15edf4f657ab553c84c5d1 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@kernel.org>
Date: Mon, 27 May 2024 17:50:54 +0300
Subject: [PATCH] fixup

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm2-cmd.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 1e856259219e..e1013762741a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -294,28 +294,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	if (!num_bytes || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
-	err = tpm2_start_auth_session(chip);
-	if (err)
-		return err;
-
 	err = tpm_buf_init(&buf, 0, 0);
-	if (err) {
-		tpm2_end_auth_session(chip);
+	if (err)
 		return err;
-	}
 
 	do {
 		tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
-		tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
-						| TPM2_SA_CONTINUE_SESSION,
-						NULL, 0);
 		tpm_buf_append_u16(&buf, num_bytes);
-		tpm_buf_fill_hmac_session(chip, &buf);
 		err = tpm_transmit_cmd(chip, &buf,
 				       offsetof(struct tpm2_get_random_out,
 						buffer),
 				       "attempting get random");
-		err = tpm_buf_check_hmac_response(chip, &buf, err);
 		if (err) {
 			if (err > 0)
 				err = -EIO;
@@ -345,12 +334,10 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	} while (retries-- && total < max);
 
 	tpm_buf_destroy(&buf);
-	tpm2_end_auth_session(chip);
-
 	return total ? total : -EIO;
+
 out:
 	tpm_buf_destroy(&buf);
-	tpm2_end_auth_session(chip);
 	return err;
 }
 
-- 
2.45.1

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

[Jarkko Sakkinen]

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.

BR, Jarkko

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

[Jarkko Sakkinen]

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.

BR, Jarkko

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

[Jarkko Sakkinen]

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.

Obfuscation *on purpose* by definition... Since I see such spots (liked
e.g. tpm_buf_parameters() to name another instance) sprinkled, I've
pretty much locked in the decision to limit hmac to x86_64. It is right
thing to do given not so great maturity level.

Whatever on x86_64 I'm confident we can fix for sure any issue but
cannot make such analysis on other platforms.

BR, Jarkko

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

[James Bottomley]

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/

James

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

[Jarkko Sakkinen]

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.

BR, Jarkko

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

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

[James Bottomley]

On Mon, 2024-05-27 at 22:53 +0300, 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:
[...]
> > > 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.

I'm not so sure.  The TCG simply defines it as a collection of flags
and every TPM tool set I've seen simply uses a list of flags as well. 
The original design was that the template would be in this one place
and everything else would call into it.  I think the reason all
template construction looks similar is for ease of auditing (it's easy
to get things, particularly the flags, wrong).

If it only has one use case, it should be spelled out but if someone
else would use it then it should be in the tpm.h shared header.

> The current constant does not really imply that it is for the null
> key,

Well, it isn't exactly: it's the required flag set for all primaries.

James

>  it is defined in the wrong file and has no actual legit
> documentation to go with it.

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

[Jarkko Sakkinen]

On Tue May 28, 2024 at 12:36 AM EEST, James Bottomley wrote:
> On Mon, 2024-05-27 at 22:53 +0300, 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:
> [...]
> > > > 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.
>
> I'm not so sure.  The TCG simply defines it as a collection of flags
> and every TPM tool set I've seen simply uses a list of flags as well. 
> The original design was that the template would be in this one place
> and everything else would call into it.  I think the reason all
> template construction looks similar is for ease of auditing (it's easy
> to get things, particularly the flags, wrong).
>
> If it only has one use case, it should be spelled out but if someone
> else would use it then it should be in the tpm.h shared header.

It is used only in tpm2-sessions.c and for the null key so there it
should be. And it is also lacking the associated documentation. Now
both name and context it is used is lost.

BR, Jarkko

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

[James Bottomley]

On Tue, 2024-05-28 at 02:17 +0300, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 12:36 AM EEST, James Bottomley wrote:
> > On Mon, 2024-05-27 at 22:53 +0300, 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:
> > [...]
> > > > > 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.
> > 
> > I'm not so sure.  The TCG simply defines it as a collection of
> > flags and every TPM tool set I've seen simply uses a list of flags
> > as well.  The original design was that the template would be in
> > this one place and everything else would call into it.  I think the
> > reason all template construction looks similar is for ease of
> > auditing (it's easy to get things, particularly the flags, wrong).
> > 
> > If it only has one use case, it should be spelled out but if
> > someone else would use it then it should be in the tpm.h shared
> > header.
> 
> It is used only in tpm2-sessions.c and for the null key so there it
> should be. And it is also lacking the associated documentation. Now
> both name and context it is used is lost.

The comment above the whole thing says what it is and where it comes
from:

	/*
	 * create the template.  Note: in order for userspace to
	 * verify the security of the system, it will have to create
	 * and certify this NULL primary, meaning all the template
	 * parameters will have to be identical, so conform exactly to
	 * the TCG TPM v2.0 Provisioning Guidance for the SRK ECC
	 * key H template (H has zero size unique points)
	 */

If we put the broken out flags back it's all fully documented.

James

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

[Jarkko Sakkinen]

On Tue May 28, 2024 at 2:44 AM EEST, James Bottomley wrote:
> On Tue, 2024-05-28 at 02:17 +0300, Jarkko Sakkinen wrote:
> > On Tue May 28, 2024 at 12:36 AM EEST, James Bottomley wrote:
> > > On Mon, 2024-05-27 at 22:53 +0300, 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:
> > > [...]
> > > > > > 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.
> > > 
> > > I'm not so sure.  The TCG simply defines it as a collection of
> > > flags and every TPM tool set I've seen simply uses a list of flags
> > > as well.  The original design was that the template would be in
> > > this one place and everything else would call into it.  I think the
> > > reason all template construction looks similar is for ease of
> > > auditing (it's easy to get things, particularly the flags, wrong).
> > > 
> > > If it only has one use case, it should be spelled out but if
> > > someone else would use it then it should be in the tpm.h shared
> > > header.
> > 
> > It is used only in tpm2-sessions.c and for the null key so there it
> > should be. And it is also lacking the associated documentation. Now
> > both name and context it is used is lost.
>
> The comment above the whole thing says what it is and where it comes
> from:
>
> 	/*
> 	 * create the template.  Note: in order for userspace to
> 	 * verify the security of the system, it will have to create
> 	 * and certify this NULL primary, meaning all the template
> 	 * parameters will have to be identical, so conform exactly to
> 	 * the TCG TPM v2.0 Provisioning Guidance for the SRK ECC
> 	 * key H template (H has zero size unique points)
> 	 */
>
> If we put the broken out flags back it's all fully documented.

Not the most productive conclusion when refusing to follow properly a
trivial request in the review feedback tbh.

BR, Jarkko

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

[Jarkko Sakkinen]

On Tue May 28, 2024 at 4:04 AM EEST, Jarkko Sakkinen wrote:
> On Tue May 28, 2024 at 2:44 AM EEST, James Bottomley wrote:
> > On Tue, 2024-05-28 at 02:17 +0300, Jarkko Sakkinen wrote:
> > > On Tue May 28, 2024 at 12:36 AM EEST, James Bottomley wrote:
> > > > On Mon, 2024-05-27 at 22:53 +0300, 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:
> > > > [...]
> > > > > > > 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.
> > > > 
> > > > I'm not so sure.  The TCG simply defines it as a collection of
> > > > flags and every TPM tool set I've seen simply uses a list of flags
> > > > as well.  The original design was that the template would be in
> > > > this one place and everything else would call into it.  I think the
> > > > reason all template construction looks similar is for ease of
> > > > auditing (it's easy to get things, particularly the flags, wrong).
> > > > 
> > > > If it only has one use case, it should be spelled out but if
> > > > someone else would use it then it should be in the tpm.h shared
> > > > header.
> > > 
> > > It is used only in tpm2-sessions.c and for the null key so there it
> > > should be. And it is also lacking the associated documentation. Now
> > > both name and context it is used is lost.
> >
> > The comment above the whole thing says what it is and where it comes
> > from:
> >
> > 	/*
> > 	 * create the template.  Note: in order for userspace to
> > 	 * verify the security of the system, it will have to create
> > 	 * and certify this NULL primary, meaning all the template
> > 	 * parameters will have to be identical, so conform exactly to
> > 	 * the TCG TPM v2.0 Provisioning Guidance for the SRK ECC
> > 	 * key H template (H has zero size unique points)
> > 	 */
> >
> > If we put the broken out flags back it's all fully documented.
>
> Not the most productive conclusion when refusing to follow properly a
> trivial request in the review feedback tbh.

In any case this particular constant can be revisited when otherwise
changes happen in the area. It is what it is for the time being. I
just need to use more strict and dense filter when check the patch
revisions next time.

BR, Jarkko

[PATCH 2/3] KEYS: trusted: Fix memory leak in tpm2_key_encode()

[Jarkko Sakkinen]

'scratch' is never freed. Fix this by calling kfree() in the success, and
in the error case.

Cc: stable@vger.kernel.org # +v5.13
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/trusted-keys/trusted_tpm2.c | 24 +++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index dfeec06301ce..c6882f5d094f 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -38,6 +38,7 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	u8 *end_work = scratch + SCRATCH_SIZE;
 	u8 *priv, *pub;
 	u16 priv_len, pub_len;
+	int ret;
 
 	priv_len = get_unaligned_be16(src) + 2;
 	priv = src;
@@ -57,8 +58,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 		unsigned char bool[3], *w = bool;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
-		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
-			return PTR_ERR(w);
+		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode")) {
+			ret = PTR_ERR(w);
+			goto err;
+		}
 		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
 	}
 
@@ -69,8 +72,10 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	 * trigger, so if it does there's something nefarious going on
 	 */
 	if (WARN(work - scratch + pub_len + priv_len + 14 > SCRATCH_SIZE,
-		 "BUG: scratch buffer is too small"))
-		return -EINVAL;
+		 "BUG: scratch buffer is too small")) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	work = asn1_encode_integer(work, end_work, options->keyhandle);
 	work = asn1_encode_octet_string(work, end_work, pub, pub_len);
@@ -79,10 +84,17 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
-	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed"))
-		return PTR_ERR(work1);
+	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
+		ret = PTR_ERR(work1);
+		goto err;
+	}
 
+	kfree(scratch);
 	return work1 - payload->blob;
+
+err:
+	kfree(scratch);
+	return ret;
 }
 
 struct tpm2_key_context {
-- 
2.45.1

[PATCH 3/3] KEYS: trusted: Do not use WARN when encode fails

[Jarkko Sakkinen]

When asn1_encode_sequence() fails, WARN is not the correct solution.

1. asn1_encode_sequence() is not an internal function (located
   in lib/asn1_encode.c).
2. Location is known, which makes the stack trace useless.
3. Results a crash if panic_on_warn is set.

It is also noteworthy that the use of WARN is undocumented, and it
should be avoided unless there is a carefully considered rationale to
use it.

Replace WARN with pr_err, and print the return value instead, which is
only useful piece of information.

Cc: stable@vger.kernel.org # v5.13+
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 security/keys/trusted-keys/trusted_tpm2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c6882f5d094f..8b7dd73d94c1 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -84,8 +84,9 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 	work1 = payload->blob;
 	work1 = asn1_encode_sequence(work1, work1 + sizeof(payload->blob),
 				     scratch, work - scratch);
-	if (WARN(IS_ERR(work1), "BUG: ASN.1 encoder failed")) {
+	if (IS_ERR(work1)) {
 		ret = PTR_ERR(work1);
+		pr_err("BUG: ASN.1 encoder failed with %d\n", ret);
 		goto err;
 	}
 
-- 
2.45.1