Re: [PATCH v4 08/13] tpm: Add full HMAC and encrypt/decrypt session handling code

From: Jarkko Sakkinen <jarkko@kernel.org>
To: James Bottomley <James.Bottomley@HansenPartnership.com>,
	linux-integrity@vger.kernel.org
Cc: keyrings@vger.kernel.org, Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH v4 08/13] tpm: Add full HMAC and encrypt/decrypt session handling code
Date: Sun, 23 Apr 2023 08:29:42 +0300	[thread overview]
Message-ID: <adad1047b7935b4f4d982dcd40f1fe92edcfd2d3.camel@kernel.org> (raw)
In-Reply-To: <20230403214003.32093-9-James.Bottomley@HansenPartnership.com>

On Mon, 2023-04-03 at 17:39 -0400, James Bottomley wrote:
> Add true session based HMAC authentication plus parameter decryption
> and response encryption using AES. The basic design is to segregate
> all the nasty crypto, hash and hmac code into tpm2-sessions.c and
> export a usable API.  The API first of all starts off by gaining a
> session with
> 
> tpm2_start_auth_session()
> 
> Which initiates a session with the TPM and allocates an opaque
> tpm2_auth structure to handle the session parameters.  Then the use is
> simply:
> 
> * tpm_buf_append_name() in place of the tpm_buf_append_u32 for the
>   handles
> 
> * tpm_buf_append_hmac_session() where tpm2_append_auth() would go
> 
> * tpm_buf_fill_hmac_session() called after the entire command buffer
>   is finished but before tpm_transmit_cmd() is called which computes
>   the correct HMAC and places it in the command at the correct
>   location.
> 
> Finally, after tpm_transmit_cmd() is called,
> tpm_buf_check_hmac_response() is called to check that the returned
> HMAC matched and collect the new state for the next use of the
> session, if any.
> 
> The features of the session is controlled by the session attributes
> set in tpm_buf_append_hmac_session().  If TPM2_SA_CONTINUE_SESSION is
> not specified, the session will be flushed and the tpm2_auth structure
> freed in tpm_buf_check_hmac_response(); otherwise the session may be
> used again.  Parameter encryption is specified by or'ing the flag
> TPM2_SA_DECRYPT and response encryption by or'ing the flag
> TPM2_SA_ENCRYPT.  the various encryptions will be taken care of by
> tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response()
> respectively.
> 
> To get all of this to work securely, the Kernel needs a primary key to
> encrypt the session salt to, so an EC key from the NULL seed is
> derived and its context saved in the tpm_chip structure.  The context
> is loaded on demand into an available volatile handle when
> tpm_start_auth_session() is called, but is flushed before that
> function exits to conserve handles.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # crypto API parts
> 
> ---
> 
> v2: fix memory leaks from smatch; adjust for name hash size
> v3: make tpm2_make_null_primary static
> v4: use crypto library functions
> ---
>  drivers/char/tpm/Kconfig         |   13 +
>  drivers/char/tpm/Makefile        |    1 +
>  drivers/char/tpm/tpm-buf.c       |    1 +
>  drivers/char/tpm/tpm-chip.c      |    3 +
>  drivers/char/tpm/tpm.h           |   10 +
>  drivers/char/tpm/tpm2-cmd.c      |    5 +
>  drivers/char/tpm/tpm2-sessions.c | 1158 ++++++++++++++++++++++++++++++
>  include/linux/tpm.h              |  165 +++++
>  8 files changed, 1356 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm2-sessions.c
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 927088b2c3d3..8af3afc48511 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -27,6 +27,19 @@ menuconfig TCG_TPM
>  
>  if TCG_TPM
>  
> +config TPM_BUS_SECURITY

"bus security" means nothing.

> +	bool "Use secure transactions on the TPM bus"

Neither does "secure transactions"

> +	default y
> +	select CRYPTO_ECDH
> +	select CRYPTO_LIB_AESCFB
> +	select CRYPTO_LIB_SHA256
> +	help
> +	  Setting this causes us to deploy a tamper resistent scheme
> +	  for communicating with the TPM to prevent or detect bus
> +	  snooping and iterposer attacks like TPM Genie.  Saying Y here
> +	  adds some encryption overhead to all kernel to TPM
> +	  transactions.

AFAIK, tamper resistance is part of the physical security. Software
does not provide tamper resistance. Or at least I've not heard that
term used with software before.

Please provide URL to TPM Genie because you can't assume it being
"commmon terminology". Or if you don't want to do that, remove it
from the description as it has on function in kconfig.

From that description it is impossible to say what this *actually*
does, if we assume that the reader is kernel developer/maintainer.
It is just gibberish.

To fix all this maybe to flag could be something like
TCG_TPM_HMAC_ENCRYPTION because it adds layer of protection.

The kconfig description should essentially two things:

1. The HMAC scheme very broadly.
2. Keys generated/used, e.g. it would be nice to know if there
   is a protection key for each boot cycle and that kind of stuff.

The current stuff helps no one to understand this feature.

R, Jarkko