[PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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
 	select CRYPTO_ECDH
 	select CRYPTO_LIB_AESCFB
 	select CRYPTO_LIB_SHA256
-- 
2.45.1

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[James Bottomley]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[James Bottomley]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[James Bottomley]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: enable HMAC encryption for only x86-64 and aarch64

[Jarkko Sakkinen]

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