| Message ID | 20240521130921.15028-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | tpm: enable HMAC encryption for only x86-64 and aarch64 | expand |
On Tue, 2024-05-21 at 16:09 +0300, Jarkko Sakkinen wrote: > Let's be more conservative and enable HMAC by default only for the > platforms where it immediately makes sense, i.e. x86-64 and aarch64. > This can be relaxed later on, and obviously the kconfig option can be > set even if not default on a particular arch. > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation") > Closes: > https://lore.kernel.org/linux-integrity/D1FCAPJSYLTS.R9VC1CXDCIHH@kernel.org/ > 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..19e61dcfcbbe 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 X86_64 || ARM64 My first instinct is to say that devices in hostile environments (like IoT) are likely in the most need of this. However, it is an experimental feature, so I would like to debug it first in the environments where it's expected to work, which is desktop and laptop, so I'm happy with this: Acked-by: James Bottomley <James.Bottomley@HansenPartnership.com> James
On Tue May 21, 2024 at 4:26 PM EEST, James Bottomley wrote: > On Tue, 2024-05-21 at 16:09 +0300, Jarkko Sakkinen wrote: > > Let's be more conservative and enable HMAC by default only for the > > platforms where it immediately makes sense, i.e. x86-64 and aarch64. > > This can be relaxed later on, and obviously the kconfig option can be > > set even if not default on a particular arch. > > > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation") > > Closes: > > https://lore.kernel.org/linux-integrity/D1FCAPJSYLTS.R9VC1CXDCIHH@kernel.org/ > > 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..19e61dcfcbbe 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 X86_64 || ARM64 > > My first instinct is to say that devices in hostile environments (like > IoT) are likely in the most need of this. However, it is an > experimental feature, so I would like to debug it first in the > environments where it's expected to work, which is desktop and laptop, > so I'm happy with this: > > Acked-by: James Bottomley <James.Bottomley@HansenPartnership.com> Thanks! And agreed but usually for IoT device you probably end up anyway creating somewhat tuned kconfig. In desktop default on makes most sense for the moment. I'm also willling to consider relaxing this later on. Asymmetric key patch set that I wrapped up together over the weekend was also pretty extensive test. First, it uses HMAC encryption for communication to make sure that private key is not eavesdropped. Secondly, it also roots to the null key if a parent is not given. So it covers all the basic features of the HMAC patch set. The only actual bug was a non-critical memory leak from v5.13, which consumes some dozens of bytes per power cycle in a common use case for trusted keys (PR already sent to Linus). BR, Jarkko
On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > Secondly, it also roots to the null key if a parent is not given. So > it covers all the basic features of the HMAC patch set. I don't think that can work. The key file would be wrapped to the parent and the null seed (and hence the wrapping) changes with every reboot. If you want a permanent key, it has to be in one of the accessible permanent hierarchies (storage ideally or endorsement). The spec has a mechanism for deriving the key from a permanent handle if the system doesn't have it in-place. I do have patches to use that because that's the way most sealed objects and keys are generated today. I'll post them (although there'll be a bit of fixing up to do). Regards, James
On Tue May 21, 2024 at 5:13 PM EEST, James Bottomley wrote: > On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > > Secondly, it also roots to the null key if a parent is not given. So > > it covers all the basic features of the HMAC patch set. > > I don't think that can work. The key file would be wrapped to the > parent and the null seed (and hence the wrapping) changes with every > reboot. If you want a permanent key, it has to be in one of the > accessible permanent hierarchies (storage ideally or endorsement). I'm fully aware that null seed is randomized per power cycle. The fallback was inherited from James Prestwood's original code and I decided to keep it as a testing feature, and also to test HMAC changes. If you look at the testing transcript in the cover letter, it should be obvious that a primary key is created in my basic test. BR, Jarkko
On Tue May 21, 2024 at 5:26 PM EEST, Jarkko Sakkinen wrote: > On Tue May 21, 2024 at 5:13 PM EEST, James Bottomley wrote: > > On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > > > Secondly, it also roots to the null key if a parent is not given. So > > > it covers all the basic features of the HMAC patch set. > > > > I don't think that can work. The key file would be wrapped to the > > parent and the null seed (and hence the wrapping) changes with every > > reboot. If you want a permanent key, it has to be in one of the > > accessible permanent hierarchies (storage ideally or endorsement). > > I'm fully aware that null seed is randomized per power cycle. > > The fallback was inherited from James Prestwood's original code and I > decided to keep it as a testing feature, and also to test HMAC changes. > > If you look at the testing transcript in the cover letter, it should be > obvious that a primary key is created in my basic test. I think what could be done to it in v3 would be to return -EOPNOTSUPP if parent is not defined. I.e. rationale here is that this way the empty option is still usable for something in future kernel releases. BR, Jarkko
On Tue, 2024-05-21 at 17:35 +0300, Jarkko Sakkinen wrote: > On Tue May 21, 2024 at 5:26 PM EEST, Jarkko Sakkinen wrote: > > On Tue May 21, 2024 at 5:13 PM EEST, James Bottomley wrote: > > > On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > > > > Secondly, it also roots to the null key if a parent is not > > > > given. So it covers all the basic features of the HMAC patch > > > > set. > > > > > > I don't think that can work. The key file would be wrapped to > > > the parent and the null seed (and hence the wrapping) changes > > > with every reboot. If you want a permanent key, it has to be in > > > one of the accessible permanent hierarchies (storage ideally or > > > endorsement). > > > > I'm fully aware that null seed is randomized per power cycle. OK, as long as this gets documented, I'm OK with it > > The fallback was inherited from James Prestwood's original code and > > I decided to keep it as a testing feature, and also to test HMAC > > changes. > > > > If you look at the testing transcript in the cover letter, it > > should beobvious that a primary key is created in my basic test. > > I think what could be done to it in v3 would be to return -EOPNOTSUPP > if parent is not defined. I.e. rationale here is that this way the > empty option is still usable for something in future kernel releases. You can absolutely have null derived parent keys (I use them for testing as well). However, the spec says the parent handle in that case should be TPM_RH_NULL (i.e. 0x40000007) not zero: https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html#name-parent James
On Tue May 21, 2024 at 5:35 PM EEST, Jarkko Sakkinen wrote: > On Tue May 21, 2024 at 5:26 PM EEST, Jarkko Sakkinen wrote: > > On Tue May 21, 2024 at 5:13 PM EEST, James Bottomley wrote: > > > On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > > > > Secondly, it also roots to the null key if a parent is not given. So > > > > it covers all the basic features of the HMAC patch set. > > > > > > I don't think that can work. The key file would be wrapped to the > > > parent and the null seed (and hence the wrapping) changes with every > > > reboot. If you want a permanent key, it has to be in one of the > > > accessible permanent hierarchies (storage ideally or endorsement). > > > > I'm fully aware that null seed is randomized per power cycle. > > > > The fallback was inherited from James Prestwood's original code and I > > decided to keep it as a testing feature, and also to test HMAC changes. > > > > If you look at the testing transcript in the cover letter, it should be > > obvious that a primary key is created in my basic test. > > I think what could be done to it in v3 would be to return -EOPNOTSUPP > if parent is not defined. I.e. rationale here is that this way the > empty option is still usable for something in future kernel releases. It was actually like this: if you explicitly set handle to RH_NULL. Have not used it a lot so did not recall this. That said I'd actually just take away any special substitution logic and use any handle given by the user space as it is. BR, Jarkko
On Tue May 21, 2024 at 5:56 PM EEST, James Bottomley wrote: > On Tue, 2024-05-21 at 17:35 +0300, Jarkko Sakkinen wrote: > > On Tue May 21, 2024 at 5:26 PM EEST, Jarkko Sakkinen wrote: > > > On Tue May 21, 2024 at 5:13 PM EEST, James Bottomley wrote: > > > > On Tue, 2024-05-21 at 17:02 +0300, Jarkko Sakkinen wrote: > > > > > Secondly, it also roots to the null key if a parent is not > > > > > given. So it covers all the basic features of the HMAC patch > > > > > set. > > > > > > > > I don't think that can work. The key file would be wrapped to > > > > the parent and the null seed (and hence the wrapping) changes > > > > with every reboot. If you want a permanent key, it has to be in > > > > one of the accessible permanent hierarchies (storage ideally or > > > > endorsement). > > > > > > I'm fully aware that null seed is randomized per power cycle. > > OK, as long as this gets documented, I'm OK with it > > > > The fallback was inherited from James Prestwood's original code and > > > I decided to keep it as a testing feature, and also to test HMAC > > > changes. > > > > > > If you look at the testing transcript in the cover letter, it > > > should beobvious that a primary key is created in my basic test. > > > > I think what could be done to it in v3 would be to return -EOPNOTSUPP > > if parent is not defined. I.e. rationale here is that this way the > > empty option is still usable for something in future kernel releases. > > You can absolutely have null derived parent keys (I use them for > testing as well). However, the spec says the parent handle in that > case should be TPM_RH_NULL (i.e. 0x40000007) not zero: > > https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html#name-parent Yep. I somehow recalled that it replaced 0x0 with RH_NULL but it actually checked whether the handle is RH_NULL and then loaded the null key if that was the case. BR, Jarkko
diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index e63a6a17793c..19e61dcfcbbe 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 X86_64 || ARM64 select CRYPTO_ECDH select CRYPTO_LIB_AESCFB select CRYPTO_LIB_SHA256
Let's be more conservative and enable HMAC by default only for the platforms where it immediately makes sense, i.e. x86-64 and aarch64. This can be relaxed later on, and obviously the kconfig option can be set even if not default on a particular arch. Cc: James Bottomley <James.Bottomley@HansenPartnership.com> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation") Closes: https://lore.kernel.org/linux-integrity/D1FCAPJSYLTS.R9VC1CXDCIHH@kernel.org/ Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- drivers/char/tpm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)