Re: [PATCH v3] tpm: Relocate buf->handles to appropriate place

[Jonathan McDowell <noodles@earth.li>]

On Tue, Jul 16, 2024 at 09:52:24PM +0300, Jarkko Sakkinen wrote:
> tpm_buf_append_name() has the following snippet in the beginning:
> 
> 	if (!tpm2_chip_auth(chip)) {
> 		tpm_buf_append_u32(buf, handle);
> 		/* count the number of handles in the upper bits of flags */
> 		buf->handles++;
> 		return;
> 	}
> 
> The claim in the comment is wrong, and the comment is in the wrong place
> as alignment in this case should not anyway be a concern of the call
> site. In essence the comment is  lying about the code, and thus needs to
> be adressed.
> 
> Further, 'handles' was incorrectly place to struct tpm_buf, as tpm-buf.c
> does manage its state. It is easy to grep that only piece of code that
> actually uses the field is tpm2-sessions.c.
> 
> Address the issues by moving the variable to struct tpm_chip.
> 
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> v3:
> * Reset chip->handles in the beginning of tpm2_start_auth_session()
>   so that it shows correct value, when TCG_TPM2_HMAC is enabled but
>   tpm2_sessions_init() has never been called.
> v2:
> * Was a bit more broken than I first thought, as 'handles' is only
>   useful for tpm2-sessions.c and has zero relation to tpm-buf.c.
> ---
>  drivers/char/tpm/tpm-buf.c       | 1 -
>  drivers/char/tpm/tpm2-cmd.c      | 2 +-
>  drivers/char/tpm/tpm2-sessions.c | 7 ++++---
>  include/linux/tpm.h              | 8 ++++----
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index cad0048bcc3c..d06e8e063151 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -44,7 +44,6 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>  	head->tag = cpu_to_be16(tag);
>  	head->length = cpu_to_be32(sizeof(*head));
>  	head->ordinal = cpu_to_be32(ordinal);
> -	buf->handles = 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_buf_reset);
>  
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 1e856259219e..b781e4406fc2 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -776,7 +776,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  	if (rc)
>  		goto out;
>  
> -	rc = tpm2_sessions_init(chip);
> +	/* rc = tpm2_sessions_init(chip); */

Left over from testing? Or should be removed entirely?

>  out:
>  	/*
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index d3521aadd43e..5e7c12d64ba8 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -238,8 +238,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>  
>  	if (!tpm2_chip_auth(chip)) {
>  		tpm_buf_append_u32(buf, handle);
> -		/* count the number of handles in the upper bits of flags */
> -		buf->handles++;
> +		chip->handles++;
>  		return;
>  	}
>  
> @@ -310,7 +309,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
>  
>  	if (!tpm2_chip_auth(chip)) {
>  		/* offset tells us where the sessions area begins */
> -		int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> +		int offset = chip->handles * 4 + TPM_HEADER_SIZE;
>  		u32 len = 9 + passphrase_len;
>  
>  		if (tpm_buf_length(buf) != offset) {
> @@ -963,6 +962,8 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>  	int rc;
>  	u32 null_key;
>  
> +	chip->handles = 0;
> +
>  	if (!auth) {
>  		dev_warn_once(&chip->dev, "auth session is not active\n");
>  		return 0;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index e93ee8d936a9..b664f7556494 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -202,9 +202,9 @@ struct tpm_chip {
>  	/* active locality */
>  	int locality;
>  
> +	/* handle count for session: */
> +	u8 handles;
>  #ifdef CONFIG_TCG_TPM2_HMAC
> -	/* details for communication security via sessions */
> -
>  	/* saved context for NULL seed */
>  	u8 null_key_context[TPM2_MAX_CONTEXT_SIZE];
>  	 /* name of NULL seed */
> @@ -377,7 +377,6 @@ struct tpm_buf {
>  	u32 flags;
>  	u32 length;
>  	u8 *data;
> -	u8 handles;
>  };
>  
>  enum tpm2_object_attributes {
> @@ -517,7 +516,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
>  	if (tpm2_chip_auth(chip)) {
>  		tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
>  	} else  {
> -		offset = buf->handles * 4 + TPM_HEADER_SIZE;
> +		offset = chip->handles * 4 + TPM_HEADER_SIZE;
>  		head = (struct tpm_header *)buf->data;
>  
>  		/*
> @@ -541,6 +540,7 @@ void tpm2_end_auth_session(struct tpm_chip *chip);
>  
>  static inline int tpm2_start_auth_session(struct tpm_chip *chip)
>  {
> +	chip->handles = 0;
>  	return 0;
>  }
>  static inline void tpm2_end_auth_session(struct tpm_chip *chip)
> -- 
> 2.45.2

J.

-- 
"I'm not anti-establishment, I just don't see the point." -- Matthew
Kirkwood, OxLUG mailing list.