From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>

tpm2_buf_append_auth has only single call site and most of its parameters
are redundant. Open code it to the call site. Remove illegit FIXME comment
as there is no categorized bug and replace it with more sane comment about
implementation (i.e. "non-opionated inline comment").

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 51 ++++-------------------
 1 file changed, 9 insertions(+), 42 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index c414a7006d78..8e3b283a59b2 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -198,36 +198,6 @@ int tpm2_key_priv(void *context, size_t hdrlen,
 	return 0;
 }
 
-/**
- * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
- *
- * @buf: an allocated tpm_buf instance
- * @session_handle: session handle
- * @nonce: the session nonce, may be NULL if not used
- * @nonce_len: the session nonce length, may be 0 if not used
- * @attributes: the session attributes
- * @hmac: the session HMAC or password, may be NULL if not used
- * @hmac_len: the session HMAC or password length, maybe 0 if not used
- */
-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
-				 const u8 *nonce, u16 nonce_len,
-				 u8 attributes,
-				 const u8 *hmac, u16 hmac_len)
-{
-	tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
-	tpm_buf_append_u32(buf, session_handle);
-	tpm_buf_append_u16(buf, nonce_len);
-
-	if (nonce && nonce_len)
-		tpm_buf_append(buf, nonce, nonce_len);
-
-	tpm_buf_append_u8(buf, attributes);
-	tpm_buf_append_u16(buf, hmac_len);
-
-	if (hmac && hmac_len)
-		tpm_buf_append(buf, hmac, hmac_len);
-}
-
 /**
  * tpm2_seal_trusted() - seal the payload of a trusted key
  *
@@ -507,19 +477,16 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
 					    options->blobauth_len);
 	} else {
 		/*
-		 * FIXME: The policy session was generated outside the
-		 * kernel so we don't known the nonce and thus can't
-		 * calculate a HMAC on it.  Therefore, the user can
-		 * only really use TPM2_PolicyPassword and we must
-		 * send down the plain text password, which could be
-		 * intercepted.  We can still encrypt the returned
-		 * key, but that's small comfort since the interposer
-		 * could repeat our actions with the exfiltrated
-		 * password.
+		 * The policy session is generated outside the kernel, and thus
+		 * the password will end up being unencrypted on the bus, as
+		 * HMAC nonce cannot be calculated for it.
 		 */
-		tpm2_buf_append_auth(&buf, options->policyhandle,
-				     NULL /* nonce */, 0, 0,
-				     options->blobauth, options->blobauth_len);
+		tpm_buf_append_u32(&buf, 9 + options->blobauth_len);
+		tpm_buf_append_u32(&buf, options->policyhandle);
+		tpm_buf_append_u16(&buf, 0);
+		tpm_buf_append_u8(&buf, 0);
+		tpm_buf_append_u16(&buf, options->blobauth_len);
+		tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
 		if (tpm2_chip_auth(chip)) {
 			tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
 		} else  {
-- 
2.39.5

On Mon, Sep 22, 2025 at 07:43:17PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>tpm2_buf_append_auth has only single call site and most of its parameters
>are redundant. Open code it to the call site. Remove illegit FIXME comment
>as there is no categorized bug and replace it with more sane comment about
>implementation (i.e. "non-opionated inline comment").
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>

Seems like a reasonable cleanup.

Reviewed-by: Jonathan McDowell <noodles@earth.li>

>---
> security/keys/trusted-keys/trusted_tpm2.c | 51 ++++-------------------
> 1 file changed, 9 insertions(+), 42 deletions(-)
>
>diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>index c414a7006d78..8e3b283a59b2 100644
>--- a/security/keys/trusted-keys/trusted_tpm2.c
>+++ b/security/keys/trusted-keys/trusted_tpm2.c
>@@ -198,36 +198,6 @@ int tpm2_key_priv(void *context, size_t hdrlen,
> 	return 0;
> }
>
>-/**
>- * tpm2_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
>- *
>- * @buf: an allocated tpm_buf instance
>- * @session_handle: session handle
>- * @nonce: the session nonce, may be NULL if not used
>- * @nonce_len: the session nonce length, may be 0 if not used
>- * @attributes: the session attributes
>- * @hmac: the session HMAC or password, may be NULL if not used
>- * @hmac_len: the session HMAC or password length, maybe 0 if not used
>- */
>-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
>-				 const u8 *nonce, u16 nonce_len,
>-				 u8 attributes,
>-				 const u8 *hmac, u16 hmac_len)
>-{
>-	tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
>-	tpm_buf_append_u32(buf, session_handle);
>-	tpm_buf_append_u16(buf, nonce_len);
>-
>-	if (nonce && nonce_len)
>-		tpm_buf_append(buf, nonce, nonce_len);
>-
>-	tpm_buf_append_u8(buf, attributes);
>-	tpm_buf_append_u16(buf, hmac_len);
>-
>-	if (hmac && hmac_len)
>-		tpm_buf_append(buf, hmac, hmac_len);
>-}
>-
> /**
>  * tpm2_seal_trusted() - seal the payload of a trusted key
>  *
>@@ -507,19 +477,16 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> 					    options->blobauth_len);
> 	} else {
> 		/*
>-		 * FIXME: The policy session was generated outside the
>-		 * kernel so we don't known the nonce and thus can't
>-		 * calculate a HMAC on it.  Therefore, the user can
>-		 * only really use TPM2_PolicyPassword and we must
>-		 * send down the plain text password, which could be
>-		 * intercepted.  We can still encrypt the returned
>-		 * key, but that's small comfort since the interposer
>-		 * could repeat our actions with the exfiltrated
>-		 * password.
>+		 * The policy session is generated outside the kernel, and thus
>+		 * the password will end up being unencrypted on the bus, as
>+		 * HMAC nonce cannot be calculated for it.
> 		 */
>-		tpm2_buf_append_auth(&buf, options->policyhandle,
>-				     NULL /* nonce */, 0, 0,
>-				     options->blobauth, options->blobauth_len);
>+		tpm_buf_append_u32(&buf, 9 + options->blobauth_len);
>+		tpm_buf_append_u32(&buf, options->policyhandle);
>+		tpm_buf_append_u16(&buf, 0);
>+		tpm_buf_append_u8(&buf, 0);
>+		tpm_buf_append_u16(&buf, options->blobauth_len);
>+		tpm_buf_append(&buf, options->blobauth, options->blobauth_len);
> 		if (tpm2_chip_auth(chip)) {
> 			tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
> 		} else  {
>-- 
>2.39.5
>
>

J.

-- 
If a program is useful, it must be changed.

On Wed, Sep 24, 2025 at 09:29:23AM +0100, Jonathan McDowell wrote:
> On Mon, Sep 22, 2025 at 07:43:17PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > 
> > tpm2_buf_append_auth has only single call site and most of its parameters
> > are redundant. Open code it to the call site. Remove illegit FIXME comment
> > as there is no categorized bug and replace it with more sane comment about
> > implementation (i.e. "non-opionated inline comment").
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> 
> Seems like a reasonable cleanup.
> 
> Reviewed-by: Jonathan McDowell <noodles@earth.li>

Thank you!

BTW, I applied your patches to master:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/

I did not yet bother to test them other than compilation and checkpatch
but I'm testing this and tpm_buf-series so they will get a lot of stress
testing. I'll rebase 'next' once they are actually tested (before PR).

BR, Jarkko