| Message ID | 20240519235122.3380-2-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: trusted: bug fixes | expand |
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
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
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
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
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
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
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
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
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". > >
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". > > > > >
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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(-)