[PATCH v2 0/3] Address !chip->auth

[PATCH v2 0/3] Address !chip->auth

[Jarkko Sakkinen]

Unless tpm2_sessions_init() is called, then chip->auth ends up being a null
pointer, which is ignored by authenticated sessions code. These patches aim
to fully address the bug, and hopefully still make into 6.10-rc7.

Tested on x86-64 with:

- TCG_TPM2_HMAC disabled.
- TCG_TPM2_HMAC enabled.
- TCG_TPM2_HMAC enabled, and "/* rc = tpm2_sessions_init(chip); */".

v2:
- Rebase to commit 8a9c6c40432e ("Merge tag 'io_uring-6.10-20240703' of git://git.kernel.dk/linux").
- Couple of cosmetic fixes.

Jarkko Sakkinen (3):
  tpm: Address !chip->auth in tpm2_*_auth_session()
  tpm: Address !chip->auth in tpm_buf_append_name()
  tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm2-sessions.c | 400 +++++++++++++++++--------------
 include/linux/tpm.h              |  75 ++----
 3 files changed, 245 insertions(+), 232 deletions(-)

-- 
2.45.2

[PATCH v2 1/3] tpm: Address !chip->auth in tpm2_*_auth_session()

[Jarkko Sakkinen]

Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can cause
a null derefence in tpm2_*_auth_session(). Thus, address !chip->auth in
tpm2_*_auth_session().

Cc: stable@vger.kernel.org # v6.9+
Reported-by: Stefan Berger <stefanb@linux.ibm.com>
Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Use dev_warn_once() instead of pr_warn_once().
---
 drivers/char/tpm/tpm2-sessions.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 907ac9956a78..2f1d96a5a5a7 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -824,8 +824,13 @@ EXPORT_SYMBOL(tpm_buf_check_hmac_response);
  */
 void tpm2_end_auth_session(struct tpm_chip *chip)
 {
-	tpm2_flush_context(chip, chip->auth->handle);
-	memzero_explicit(chip->auth, sizeof(*chip->auth));
+	struct tpm2_auth *auth = chip->auth;
+
+	if (!auth)
+		return;
+
+	tpm2_flush_context(chip, auth->handle);
+	memzero_explicit(auth, sizeof(*auth));
 }
 EXPORT_SYMBOL(tpm2_end_auth_session);
 
@@ -907,6 +912,11 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	int rc;
 	u32 null_key;
 
+	if (!auth) {
+		dev_warn_once(&chip->dev, "auth session is not active\n");
+		return 0;
+	}
+
 	rc = tpm2_load_null(chip, &null_key);
 	if (rc)
 		goto out;
-- 
2.45.2

[PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[Jarkko Sakkinen]

Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
cause a null derefence in tpm_buf_append_name().  Thus, address
!chip->auth in tpm_buf_append_name() and remove the fallback
implementation for !TCG_TPM2_HMAC.

Cc: stable@vger.kernel.org # v6.9+
Reported-by: Stefan Berger <stefanb@linux.ibm.com>
Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
Fixes: d0a25bb961e6 ("tpm: Add HMAC session name/handle append")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/Makefile        |   2 +-
 drivers/char/tpm/tpm2-sessions.c | 205 ++++++++++++++++---------------
 include/linux/tpm.h              |  16 +--
 3 files changed, 113 insertions(+), 110 deletions(-)

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 4c695b0388f3..9bb142c75243 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -16,8 +16,8 @@ tpm-y += eventlog/common.o
 tpm-y += eventlog/tpm1.o
 tpm-y += eventlog/tpm2.o
 tpm-y += tpm-buf.o
+tpm-y += tpm2-sessions.o
 
-tpm-$(CONFIG_TCG_TPM2_HMAC) += tpm2-sessions.o
 tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
 tpm-$(CONFIG_EFI) += eventlog/efi.o
 tpm-$(CONFIG_OF) += eventlog/of.o
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 2f1d96a5a5a7..06d0f10a2301 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -163,6 +163,112 @@ static u8 name_size(const u8 *name)
 	return size_map[alg] + 2;
 }
 
+static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
+{
+	struct tpm_header *head = (struct tpm_header *)buf->data;
+	off_t offset = TPM_HEADER_SIZE;
+	u32 tot_len = be32_to_cpu(head->length);
+	u32 val;
+
+	/* we're starting after the header so adjust the length */
+	tot_len -= TPM_HEADER_SIZE;
+
+	/* skip public */
+	val = tpm_buf_read_u16(buf, &offset);
+	if (val > tot_len)
+		return -EINVAL;
+	offset += val;
+	/* name */
+	val = tpm_buf_read_u16(buf, &offset);
+	if (val != name_size(&buf->data[offset]))
+		return -EINVAL;
+	memcpy(name, &buf->data[offset], val);
+	/* forget the rest */
+	return 0;
+}
+
+static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
+{
+	struct tpm_buf buf;
+	int rc;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_READ_PUBLIC);
+	if (rc)
+		return rc;
+
+	tpm_buf_append_u32(&buf, handle);
+	rc = tpm_transmit_cmd(chip, &buf, 0, "read public");
+	if (rc == TPM2_RC_SUCCESS)
+		rc = tpm2_parse_read_public(name, &buf);
+
+	tpm_buf_destroy(&buf);
+
+	return rc;
+}
+
+/**
+ * tpm_buf_append_name() - add a handle area to the buffer
+ * @chip: the TPM chip structure
+ * @buf: The buffer to be appended
+ * @handle: The handle to be appended
+ * @name: The name of the handle (may be NULL)
+ *
+ * In order to compute session HMACs, we need to know the names of the
+ * objects pointed to by the handles.  For most objects, this is simply
+ * the actual 4 byte handle or an empty buf (in these cases @name
+ * should be NULL) but for volatile objects, permanent objects and NV
+ * areas, the name is defined as the hash (according to the name
+ * algorithm which should be set to sha256) of the public area to
+ * which the two byte algorithm id has been appended.  For these
+ * objects, the @name pointer should point to this.  If a name is
+ * required but @name is NULL, then TPM2_ReadPublic() will be called
+ * on the handle to obtain the name.
+ *
+ * As with most tpm_buf operations, success is assumed because failure
+ * will be caused by an incorrect programming model and indicated by a
+ * kernel message.
+ */
+void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
+			 u32 handle, u8 *name)
+{
+	enum tpm2_mso_type mso = tpm2_handle_mso(handle);
+	struct tpm2_auth *auth = chip->auth;
+	int slot;
+
+	if (!chip->auth) {
+		tpm_buf_append_u32(buf, handle);
+		/* count the number of handles in the upper bits of flags */
+		buf->handles++;
+		return;
+	}
+
+	slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
+	if (slot >= AUTH_MAX_NAMES) {
+		dev_err(&chip->dev, "TPM: too many handles\n");
+		return;
+	}
+	WARN(auth->session != tpm_buf_length(buf),
+	     "name added in wrong place\n");
+	tpm_buf_append_u32(buf, handle);
+	auth->session += 4;
+
+	if (mso == TPM2_MSO_PERSISTENT ||
+	    mso == TPM2_MSO_VOLATILE ||
+	    mso == TPM2_MSO_NVRAM) {
+		if (!name)
+			tpm2_read_public(chip, handle, auth->name[slot]);
+	} else {
+		if (name)
+			dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
+	}
+
+	auth->name_h[slot] = handle;
+	if (name)
+		memcpy(auth->name[slot], name, name_size(name));
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_name);
+
+#ifdef CONFIG_TCG_TPM2_HMAC
 /*
  * It turns out the crypto hmac(sha256) is hard for us to consume
  * because it assumes a fixed key and the TPM seems to change the key
@@ -567,104 +673,6 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
 }
 EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
 
-static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
-{
-	struct tpm_header *head = (struct tpm_header *)buf->data;
-	off_t offset = TPM_HEADER_SIZE;
-	u32 tot_len = be32_to_cpu(head->length);
-	u32 val;
-
-	/* we're starting after the header so adjust the length */
-	tot_len -= TPM_HEADER_SIZE;
-
-	/* skip public */
-	val = tpm_buf_read_u16(buf, &offset);
-	if (val > tot_len)
-		return -EINVAL;
-	offset += val;
-	/* name */
-	val = tpm_buf_read_u16(buf, &offset);
-	if (val != name_size(&buf->data[offset]))
-		return -EINVAL;
-	memcpy(name, &buf->data[offset], val);
-	/* forget the rest */
-	return 0;
-}
-
-static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
-{
-	struct tpm_buf buf;
-	int rc;
-
-	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_READ_PUBLIC);
-	if (rc)
-		return rc;
-
-	tpm_buf_append_u32(&buf, handle);
-	rc = tpm_transmit_cmd(chip, &buf, 0, "read public");
-	if (rc == TPM2_RC_SUCCESS)
-		rc = tpm2_parse_read_public(name, &buf);
-
-	tpm_buf_destroy(&buf);
-
-	return rc;
-}
-
-/**
- * tpm_buf_append_name() - add a handle area to the buffer
- * @chip: the TPM chip structure
- * @buf: The buffer to be appended
- * @handle: The handle to be appended
- * @name: The name of the handle (may be NULL)
- *
- * In order to compute session HMACs, we need to know the names of the
- * objects pointed to by the handles.  For most objects, this is simply
- * the actual 4 byte handle or an empty buf (in these cases @name
- * should be NULL) but for volatile objects, permanent objects and NV
- * areas, the name is defined as the hash (according to the name
- * algorithm which should be set to sha256) of the public area to
- * which the two byte algorithm id has been appended.  For these
- * objects, the @name pointer should point to this.  If a name is
- * required but @name is NULL, then TPM2_ReadPublic() will be called
- * on the handle to obtain the name.
- *
- * As with most tpm_buf operations, success is assumed because failure
- * will be caused by an incorrect programming model and indicated by a
- * kernel message.
- */
-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
-			 u32 handle, u8 *name)
-{
-	enum tpm2_mso_type mso = tpm2_handle_mso(handle);
-	struct tpm2_auth *auth = chip->auth;
-	int slot;
-
-	slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE)/4;
-	if (slot >= AUTH_MAX_NAMES) {
-		dev_err(&chip->dev, "TPM: too many handles\n");
-		return;
-	}
-	WARN(auth->session != tpm_buf_length(buf),
-	     "name added in wrong place\n");
-	tpm_buf_append_u32(buf, handle);
-	auth->session += 4;
-
-	if (mso == TPM2_MSO_PERSISTENT ||
-	    mso == TPM2_MSO_VOLATILE ||
-	    mso == TPM2_MSO_NVRAM) {
-		if (!name)
-			tpm2_read_public(chip, handle, auth->name[slot]);
-	} else {
-		if (name)
-			dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
-	}
-
-	auth->name_h[slot] = handle;
-	if (name)
-		memcpy(auth->name[slot], name, name_size(name));
-}
-EXPORT_SYMBOL(tpm_buf_append_name);
-
 /**
  * tpm_buf_check_hmac_response() - check the TPM return HMAC for correctness
  * @chip: the TPM chip structure
@@ -1311,3 +1319,4 @@ int tpm2_sessions_init(struct tpm_chip *chip)
 
 	return rc;
 }
+#endif /* CONFIG_TCG_TPM2_HMAC */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 21a67dc9efe8..2844fea4a12a 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -211,8 +211,8 @@ struct tpm_chip {
 	u8 null_key_name[TPM2_NAME_SIZE];
 	u8 null_ec_key_x[EC_PT_SZ];
 	u8 null_ec_key_y[EC_PT_SZ];
-	struct tpm2_auth *auth;
 #endif
+	struct tpm2_auth *auth;
 };
 
 #define TPM_HEADER_SIZE		10
@@ -490,11 +490,13 @@ static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle)
 {
 }
 #endif
-#ifdef CONFIG_TCG_TPM2_HMAC
 
-int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
 			 u32 handle, u8 *name);
+
+#ifdef CONFIG_TCG_TPM2_HMAC
+
+int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
 				 u8 attributes, u8 *passphrase,
 				 int passphraselen);
@@ -521,14 +523,6 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
 static inline void tpm2_end_auth_session(struct tpm_chip *chip)
 {
 }
-static inline void tpm_buf_append_name(struct tpm_chip *chip,
-				       struct tpm_buf *buf,
-				       u32 handle, u8 *name)
-{
-	tpm_buf_append_u32(buf, handle);
-	/* count the number of handles in the upper bits of flags */
-	buf->handles++;
-}
 static inline void tpm_buf_append_hmac_session(struct tpm_chip *chip,
 					       struct tpm_buf *buf,
 					       u8 attributes, u8 *passphrase,
-- 
2.45.2

[PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Jarkko Sakkinen]

Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
cause a null derefence in tpm_buf_hmac_session*().  Thus, address
!chip->auth in tpm_buf_hmac_session*() and remove the fallback
implementation for !TCG_TPM2_HMAC.

Cc: stable@vger.kernel.org # v6.9+
Reported-by: Stefan Berger <stefanb@linux.ibm.com>
Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v2:
- Use auth in place of chip->auth.
---
 drivers/char/tpm/tpm2-sessions.c | 181 ++++++++++++++++++-------------
 include/linux/tpm.h              |  67 ++++--------
 2 files changed, 124 insertions(+), 124 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 06d0f10a2301..304247090b56 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -268,6 +268,105 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append_name);
 
+/**
+ * tpm_buf_append_hmac_session() - Append a TPM session element
+ * @chip: the TPM chip structure
+ * @buf: The buffer to be appended
+ * @attributes: The session attributes
+ * @passphrase: The session authority (NULL if none)
+ * @passphrase_len: The length of the session authority (0 if none)
+ *
+ * This fills in a session structure in the TPM command buffer, except
+ * for the HMAC which cannot be computed until the command buffer is
+ * complete.  The type of session is controlled by the @attributes,
+ * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the
+ * session won't terminate after tpm_buf_check_hmac_response(),
+ * TPM2_SA_DECRYPT which means this buffers first parameter should be
+ * encrypted with a session key and TPM2_SA_ENCRYPT, which means the
+ * response buffer's first parameter needs to be decrypted (confusing,
+ * but the defines are written from the point of view of the TPM).
+ *
+ * Any session appended by this command must be finalized by calling
+ * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect
+ * and the TPM will reject the command.
+ *
+ * As with most tpm_buf operations, success is assumed because failure
+ * will be caused by an incorrect programming model and indicated by a
+ * kernel message.
+ */
+void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
+				 u8 attributes, u8 *passphrase,
+				 int passphrase_len)
+{
+	struct tpm2_auth *auth = chip->auth;
+	u8 nonce[SHA256_DIGEST_SIZE];
+	u32 len;
+
+	if (!auth) {
+		/* offset tells us where the sessions area begins */
+		int offset = buf->handles * 4 + TPM_HEADER_SIZE;
+		u32 len = 9 + passphrase_len;
+
+		if (tpm_buf_length(buf) != offset) {
+			/* not the first session so update the existing length */
+			len += get_unaligned_be32(&buf->data[offset]);
+			put_unaligned_be32(len, &buf->data[offset]);
+		} else {
+			tpm_buf_append_u32(buf, len);
+		}
+		/* auth handle */
+		tpm_buf_append_u32(buf, TPM2_RS_PW);
+		/* nonce */
+		tpm_buf_append_u16(buf, 0);
+		/* attributes */
+		tpm_buf_append_u8(buf, 0);
+		/* passphrase */
+		tpm_buf_append_u16(buf, passphrase_len);
+		tpm_buf_append(buf, passphrase, passphrase_len);
+		return;
+	}
+
+	/*
+	 * The Architecture Guide requires us to strip trailing zeros
+	 * before computing the HMAC
+	 */
+	while (passphrase && passphrase_len > 0 && passphrase[passphrase_len - 1] == '\0')
+		passphrase_len--;
+
+	auth->attrs = attributes;
+	auth->passphrase_len = passphrase_len;
+	if (passphrase_len)
+		memcpy(auth->passphrase, passphrase, passphrase_len);
+
+	if (auth->session != tpm_buf_length(buf)) {
+		/* we're not the first session */
+		len = get_unaligned_be32(&buf->data[auth->session]);
+		if (4 + len + auth->session != tpm_buf_length(buf)) {
+			WARN(1, "session length mismatch, cannot append");
+			return;
+		}
+
+		/* add our new session */
+		len += 9 + 2 * SHA256_DIGEST_SIZE;
+		put_unaligned_be32(len, &buf->data[auth->session]);
+	} else {
+		tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE);
+	}
+
+	/* random number for our nonce */
+	get_random_bytes(nonce, sizeof(nonce));
+	memcpy(auth->our_nonce, nonce, sizeof(nonce));
+	tpm_buf_append_u32(buf, auth->handle);
+	/* our new nonce */
+	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
+	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
+	tpm_buf_append_u8(buf, auth->attrs);
+	/* and put a placeholder for the hmac */
+	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
+	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_hmac_session);
+
 #ifdef CONFIG_TCG_TPM2_HMAC
 /*
  * It turns out the crypto hmac(sha256) is hard for us to consume
@@ -449,82 +548,6 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
 	crypto_free_kpp(kpp);
 }
 
-/**
- * tpm_buf_append_hmac_session() - Append a TPM session element
- * @chip: the TPM chip structure
- * @buf: The buffer to be appended
- * @attributes: The session attributes
- * @passphrase: The session authority (NULL if none)
- * @passphrase_len: The length of the session authority (0 if none)
- *
- * This fills in a session structure in the TPM command buffer, except
- * for the HMAC which cannot be computed until the command buffer is
- * complete.  The type of session is controlled by the @attributes,
- * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the
- * session won't terminate after tpm_buf_check_hmac_response(),
- * TPM2_SA_DECRYPT which means this buffers first parameter should be
- * encrypted with a session key and TPM2_SA_ENCRYPT, which means the
- * response buffer's first parameter needs to be decrypted (confusing,
- * but the defines are written from the point of view of the TPM).
- *
- * Any session appended by this command must be finalized by calling
- * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect
- * and the TPM will reject the command.
- *
- * As with most tpm_buf operations, success is assumed because failure
- * will be caused by an incorrect programming model and indicated by a
- * kernel message.
- */
-void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
-				 u8 attributes, u8 *passphrase,
-				 int passphrase_len)
-{
-	u8 nonce[SHA256_DIGEST_SIZE];
-	u32 len;
-	struct tpm2_auth *auth = chip->auth;
-
-	/*
-	 * The Architecture Guide requires us to strip trailing zeros
-	 * before computing the HMAC
-	 */
-	while (passphrase && passphrase_len > 0
-	       && passphrase[passphrase_len - 1] == '\0')
-		passphrase_len--;
-
-	auth->attrs = attributes;
-	auth->passphrase_len = passphrase_len;
-	if (passphrase_len)
-		memcpy(auth->passphrase, passphrase, passphrase_len);
-
-	if (auth->session != tpm_buf_length(buf)) {
-		/* we're not the first session */
-		len = get_unaligned_be32(&buf->data[auth->session]);
-		if (4 + len + auth->session != tpm_buf_length(buf)) {
-			WARN(1, "session length mismatch, cannot append");
-			return;
-		}
-
-		/* add our new session */
-		len += 9 + 2 * SHA256_DIGEST_SIZE;
-		put_unaligned_be32(len, &buf->data[auth->session]);
-	} else {
-		tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE);
-	}
-
-	/* random number for our nonce */
-	get_random_bytes(nonce, sizeof(nonce));
-	memcpy(auth->our_nonce, nonce, sizeof(nonce));
-	tpm_buf_append_u32(buf, auth->handle);
-	/* our new nonce */
-	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
-	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
-	tpm_buf_append_u8(buf, auth->attrs);
-	/* and put a placeholder for the hmac */
-	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
-	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
-}
-EXPORT_SYMBOL(tpm_buf_append_hmac_session);
-
 /**
  * tpm_buf_fill_hmac_session() - finalize the session HMAC
  * @chip: the TPM chip structure
@@ -555,6 +578,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
 	u8 cphash[SHA256_DIGEST_SIZE];
 	struct sha256_state sctx;
 
+	if (!auth)
+		return;
+
 	/* save the command code in BE format */
 	auth->ordinal = head->ordinal;
 
@@ -713,6 +739,9 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
 	u32 cc = be32_to_cpu(auth->ordinal);
 	int parm_len, len, i, handles;
 
+	if (!auth)
+		return rc;
+
 	if (auth->session >= TPM_HEADER_SIZE) {
 		WARN(1, "tpm session not filled correctly\n");
 		goto out;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 2844fea4a12a..912fd0d2646d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -493,22 +493,35 @@ static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle)
 
 void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
 			 u32 handle, u8 *name);
-
-#ifdef CONFIG_TCG_TPM2_HMAC
-
-int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
 				 u8 attributes, u8 *passphrase,
 				 int passphraselen);
+
 static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
 						   struct tpm_buf *buf,
 						   u8 attributes,
 						   u8 *passphrase,
 						   int passphraselen)
 {
-	tpm_buf_append_hmac_session(chip, buf, attributes, passphrase,
-				    passphraselen);
+	struct tpm_header *head = (struct tpm_header *)buf->data;
+	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
+
+	if (chip->auth) {
+		tpm_buf_append_hmac_session(chip, buf, attributes, passphrase,
+					    passphraselen);
+	} else  {
+		/*
+		 * If the only sessions are optional, the command tag must change to
+		 * TPM2_ST_NO_SESSIONS.
+		 */
+		if (tpm_buf_length(buf) == offset)
+			head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
+	}
 }
+
+#ifdef CONFIG_TCG_TPM2_HMAC
+
+int tpm2_start_auth_session(struct tpm_chip *chip);
 void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
 int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
 				int rc);
@@ -523,48 +536,6 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
 static inline void tpm2_end_auth_session(struct tpm_chip *chip)
 {
 }
-static inline void tpm_buf_append_hmac_session(struct tpm_chip *chip,
-					       struct tpm_buf *buf,
-					       u8 attributes, u8 *passphrase,
-					       int passphraselen)
-{
-	/* offset tells us where the sessions area begins */
-	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
-	u32 len = 9 + passphraselen;
-
-	if (tpm_buf_length(buf) != offset) {
-		/* not the first session so update the existing length */
-		len += get_unaligned_be32(&buf->data[offset]);
-		put_unaligned_be32(len, &buf->data[offset]);
-	} else {
-		tpm_buf_append_u32(buf, len);
-	}
-	/* auth handle */
-	tpm_buf_append_u32(buf, TPM2_RS_PW);
-	/* nonce */
-	tpm_buf_append_u16(buf, 0);
-	/* attributes */
-	tpm_buf_append_u8(buf, 0);
-	/* passphrase */
-	tpm_buf_append_u16(buf, passphraselen);
-	tpm_buf_append(buf, passphrase, passphraselen);
-}
-static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
-						   struct tpm_buf *buf,
-						   u8 attributes,
-						   u8 *passphrase,
-						   int passphraselen)
-{
-	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
-	struct tpm_header *head = (struct tpm_header *) buf->data;
-
-	/*
-	 * if the only sessions are optional, the command tag
-	 * must change to TPM2_ST_NO_SESSIONS
-	 */
-	if (tpm_buf_length(buf) == offset)
-		head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
-}
 static inline void tpm_buf_fill_hmac_session(struct tpm_chip *chip,
 					     struct tpm_buf *buf)
 {
-- 
2.45.2

Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[James Bottomley]

On Wed, 2024-07-03 at 21:24 +0300, Jarkko Sakkinen wrote:
[...]
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 21a67dc9efe8..2844fea4a12a 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -211,8 +211,8 @@ struct tpm_chip {
>         u8 null_key_name[TPM2_NAME_SIZE];
>         u8 null_ec_key_x[EC_PT_SZ];
>         u8 null_ec_key_y[EC_PT_SZ];
> -       struct tpm2_auth *auth;
>  #endif
> +       struct tpm2_auth *auth;
>  };

Since auth should only be present if CONFIG_TCG_TPM2_HMAC this is
clearly an undesirable thing to do.  I think you did it because in a
later patch you want to collapse the hmac sessions to use a single
routine, but you can make that check with the preprocessor __and
function defined in kconfig.h:

if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))

Which will become 0 if the config is not enabled and chip->auth if it
is, thus eliminating the code in the former case while not causing the
compiler to complain about chip->auth not being defined even if it's
under the config parameter.

James

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Stefan Berger]

On 7/3/24 14:24, Jarkko Sakkinen wrote:
> Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can

Doesn't tpm_chip_register() need to be called by all drivers? This 
function then calls tpm_chip_bootstrap().

> cause a null derefence in tpm_buf_hmac_session*().  Thus, address
> !chip->auth in tpm_buf_hmac_session*() and remove the fallback
> implementation for !TCG_TPM2_HMAC.
> 
> Cc: stable@vger.kernel.org # v6.9+
> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
> Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
> Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

I applied this series now but it doesn't solve the reported problem. The 
error message is gone but the feature can still be enabled 
(CONFIG_TCG_TPM2_HMAC=y) but is unlikely actually doing what it is 
promising to do with this config option. So you either still have to 
apply my patch, James's patch, or your intended "depends on 
!TCG_IBMVTPM" patch.

[    1.449673] tpm_ibmvtpm 5000: CRQ initialized
[    1.449726] tpm_ibmvtpm 5000: CRQ initialization completed
[    2.483218] tpm tpm0: auth session is not active


    Stefan
> ---
> v2:
> - Use auth in place of chip->auth.
> ---
>   drivers/char/tpm/tpm2-sessions.c | 181 ++++++++++++++++++-------------
>   include/linux/tpm.h              |  67 ++++--------
>   2 files changed, 124 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 06d0f10a2301..304247090b56 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -268,6 +268,105 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>   }
>   EXPORT_SYMBOL_GPL(tpm_buf_append_name);
>   
> +/**
> + * tpm_buf_append_hmac_session() - Append a TPM session element
> + * @chip: the TPM chip structure
> + * @buf: The buffer to be appended
> + * @attributes: The session attributes
> + * @passphrase: The session authority (NULL if none)
> + * @passphrase_len: The length of the session authority (0 if none)
> + *
> + * This fills in a session structure in the TPM command buffer, except
> + * for the HMAC which cannot be computed until the command buffer is
> + * complete.  The type of session is controlled by the @attributes,
> + * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the
> + * session won't terminate after tpm_buf_check_hmac_response(),
> + * TPM2_SA_DECRYPT which means this buffers first parameter should be
> + * encrypted with a session key and TPM2_SA_ENCRYPT, which means the
> + * response buffer's first parameter needs to be decrypted (confusing,
> + * but the defines are written from the point of view of the TPM).
> + *
> + * Any session appended by this command must be finalized by calling
> + * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect
> + * and the TPM will reject the command.
> + *
> + * As with most tpm_buf operations, success is assumed because failure
> + * will be caused by an incorrect programming model and indicated by a
> + * kernel message.
> + */
> +void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> +				 u8 attributes, u8 *passphrase,
> +				 int passphrase_len)
> +{
> +	struct tpm2_auth *auth = chip->auth;
> +	u8 nonce[SHA256_DIGEST_SIZE];
> +	u32 len;
> +
> +	if (!auth) {
> +		/* offset tells us where the sessions area begins */
> +		int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> +		u32 len = 9 + passphrase_len;
> +
> +		if (tpm_buf_length(buf) != offset) {
> +			/* not the first session so update the existing length */
> +			len += get_unaligned_be32(&buf->data[offset]);
> +			put_unaligned_be32(len, &buf->data[offset]);
> +		} else {
> +			tpm_buf_append_u32(buf, len);
> +		}
> +		/* auth handle */
> +		tpm_buf_append_u32(buf, TPM2_RS_PW);
> +		/* nonce */
> +		tpm_buf_append_u16(buf, 0);
> +		/* attributes */
> +		tpm_buf_append_u8(buf, 0);
> +		/* passphrase */
> +		tpm_buf_append_u16(buf, passphrase_len);
> +		tpm_buf_append(buf, passphrase, passphrase_len);
> +		return;
> +	}
> +
> +	/*
> +	 * The Architecture Guide requires us to strip trailing zeros
> +	 * before computing the HMAC
> +	 */
> +	while (passphrase && passphrase_len > 0 && passphrase[passphrase_len - 1] == '\0')
> +		passphrase_len--;
> +
> +	auth->attrs = attributes;
> +	auth->passphrase_len = passphrase_len;
> +	if (passphrase_len)
> +		memcpy(auth->passphrase, passphrase, passphrase_len);
> +
> +	if (auth->session != tpm_buf_length(buf)) {
> +		/* we're not the first session */
> +		len = get_unaligned_be32(&buf->data[auth->session]);
> +		if (4 + len + auth->session != tpm_buf_length(buf)) {
> +			WARN(1, "session length mismatch, cannot append");
> +			return;
> +		}
> +
> +		/* add our new session */
> +		len += 9 + 2 * SHA256_DIGEST_SIZE;
> +		put_unaligned_be32(len, &buf->data[auth->session]);
> +	} else {
> +		tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE);
> +	}
> +
> +	/* random number for our nonce */
> +	get_random_bytes(nonce, sizeof(nonce));
> +	memcpy(auth->our_nonce, nonce, sizeof(nonce));
> +	tpm_buf_append_u32(buf, auth->handle);
> +	/* our new nonce */
> +	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
> +	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
> +	tpm_buf_append_u8(buf, auth->attrs);
> +	/* and put a placeholder for the hmac */
> +	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
> +	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_append_hmac_session);
> +
>   #ifdef CONFIG_TCG_TPM2_HMAC
>   /*
>    * It turns out the crypto hmac(sha256) is hard for us to consume
> @@ -449,82 +548,6 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>   	crypto_free_kpp(kpp);
>   }
>   
> -/**
> - * tpm_buf_append_hmac_session() - Append a TPM session element
> - * @chip: the TPM chip structure
> - * @buf: The buffer to be appended
> - * @attributes: The session attributes
> - * @passphrase: The session authority (NULL if none)
> - * @passphrase_len: The length of the session authority (0 if none)
> - *
> - * This fills in a session structure in the TPM command buffer, except
> - * for the HMAC which cannot be computed until the command buffer is
> - * complete.  The type of session is controlled by the @attributes,
> - * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the
> - * session won't terminate after tpm_buf_check_hmac_response(),
> - * TPM2_SA_DECRYPT which means this buffers first parameter should be
> - * encrypted with a session key and TPM2_SA_ENCRYPT, which means the
> - * response buffer's first parameter needs to be decrypted (confusing,
> - * but the defines are written from the point of view of the TPM).
> - *
> - * Any session appended by this command must be finalized by calling
> - * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect
> - * and the TPM will reject the command.
> - *
> - * As with most tpm_buf operations, success is assumed because failure
> - * will be caused by an incorrect programming model and indicated by a
> - * kernel message.
> - */
> -void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> -				 u8 attributes, u8 *passphrase,
> -				 int passphrase_len)
> -{
> -	u8 nonce[SHA256_DIGEST_SIZE];
> -	u32 len;
> -	struct tpm2_auth *auth = chip->auth;
> -
> -	/*
> -	 * The Architecture Guide requires us to strip trailing zeros
> -	 * before computing the HMAC
> -	 */
> -	while (passphrase && passphrase_len > 0
> -	       && passphrase[passphrase_len - 1] == '\0')
> -		passphrase_len--;
> -
> -	auth->attrs = attributes;
> -	auth->passphrase_len = passphrase_len;
> -	if (passphrase_len)
> -		memcpy(auth->passphrase, passphrase, passphrase_len);
> -
> -	if (auth->session != tpm_buf_length(buf)) {
> -		/* we're not the first session */
> -		len = get_unaligned_be32(&buf->data[auth->session]);
> -		if (4 + len + auth->session != tpm_buf_length(buf)) {
> -			WARN(1, "session length mismatch, cannot append");
> -			return;
> -		}
> -
> -		/* add our new session */
> -		len += 9 + 2 * SHA256_DIGEST_SIZE;
> -		put_unaligned_be32(len, &buf->data[auth->session]);
> -	} else {
> -		tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE);
> -	}
> -
> -	/* random number for our nonce */
> -	get_random_bytes(nonce, sizeof(nonce));
> -	memcpy(auth->our_nonce, nonce, sizeof(nonce));
> -	tpm_buf_append_u32(buf, auth->handle);
> -	/* our new nonce */
> -	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
> -	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
> -	tpm_buf_append_u8(buf, auth->attrs);
> -	/* and put a placeholder for the hmac */
> -	tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
> -	tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
> -}
> -EXPORT_SYMBOL(tpm_buf_append_hmac_session);
> -
>   /**
>    * tpm_buf_fill_hmac_session() - finalize the session HMAC
>    * @chip: the TPM chip structure
> @@ -555,6 +578,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
>   	u8 cphash[SHA256_DIGEST_SIZE];
>   	struct sha256_state sctx;
>   
> +	if (!auth)
> +		return;
> +
>   	/* save the command code in BE format */
>   	auth->ordinal = head->ordinal;
>   
> @@ -713,6 +739,9 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>   	u32 cc = be32_to_cpu(auth->ordinal);
>   	int parm_len, len, i, handles;
>   
> +	if (!auth)
> +		return rc;
> +
>   	if (auth->session >= TPM_HEADER_SIZE) {
>   		WARN(1, "tpm session not filled correctly\n");
>   		goto out;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 2844fea4a12a..912fd0d2646d 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -493,22 +493,35 @@ static inline void tpm_buf_append_empty_auth(struct tpm_buf *buf, u32 handle)
>   
>   void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>   			 u32 handle, u8 *name);
> -
> -#ifdef CONFIG_TCG_TPM2_HMAC
> -
> -int tpm2_start_auth_session(struct tpm_chip *chip);
>   void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
>   				 u8 attributes, u8 *passphrase,
>   				 int passphraselen);
> +
>   static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
>   						   struct tpm_buf *buf,
>   						   u8 attributes,
>   						   u8 *passphrase,
>   						   int passphraselen)
>   {
> -	tpm_buf_append_hmac_session(chip, buf, attributes, passphrase,
> -				    passphraselen);
> +	struct tpm_header *head = (struct tpm_header *)buf->data;
> +	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> +
> +	if (chip->auth) {
> +		tpm_buf_append_hmac_session(chip, buf, attributes, passphrase,
> +					    passphraselen);
> +	} else  {
> +		/*
> +		 * If the only sessions are optional, the command tag must change to
> +		 * TPM2_ST_NO_SESSIONS.
> +		 */
> +		if (tpm_buf_length(buf) == offset)
> +			head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> +	}
>   }
> +
> +#ifdef CONFIG_TCG_TPM2_HMAC
> +
> +int tpm2_start_auth_session(struct tpm_chip *chip);
>   void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf);
>   int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>   				int rc);
> @@ -523,48 +536,6 @@ static inline int tpm2_start_auth_session(struct tpm_chip *chip)
>   static inline void tpm2_end_auth_session(struct tpm_chip *chip)
>   {
>   }
> -static inline void tpm_buf_append_hmac_session(struct tpm_chip *chip,
> -					       struct tpm_buf *buf,
> -					       u8 attributes, u8 *passphrase,
> -					       int passphraselen)
> -{
> -	/* offset tells us where the sessions area begins */
> -	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> -	u32 len = 9 + passphraselen;
> -
> -	if (tpm_buf_length(buf) != offset) {
> -		/* not the first session so update the existing length */
> -		len += get_unaligned_be32(&buf->data[offset]);
> -		put_unaligned_be32(len, &buf->data[offset]);
> -	} else {
> -		tpm_buf_append_u32(buf, len);
> -	}
> -	/* auth handle */
> -	tpm_buf_append_u32(buf, TPM2_RS_PW);
> -	/* nonce */
> -	tpm_buf_append_u16(buf, 0);
> -	/* attributes */
> -	tpm_buf_append_u8(buf, 0);
> -	/* passphrase */
> -	tpm_buf_append_u16(buf, passphraselen);
> -	tpm_buf_append(buf, passphrase, passphraselen);
> -}
> -static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> -						   struct tpm_buf *buf,
> -						   u8 attributes,
> -						   u8 *passphrase,
> -						   int passphraselen)
> -{
> -	int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> -	struct tpm_header *head = (struct tpm_header *) buf->data;
> -
> -	/*
> -	 * if the only sessions are optional, the command tag
> -	 * must change to TPM2_ST_NO_SESSIONS
> -	 */
> -	if (tpm_buf_length(buf) == offset)
> -		head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> -}
>   static inline void tpm_buf_fill_hmac_session(struct tpm_chip *chip,
>   					     struct tpm_buf *buf)
>   {

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Jarkko Sakkinen]

On Thu Jul 4, 2024 at 4:56 AM EEST, Stefan Berger wrote:
>
>
> On 7/3/24 14:24, Jarkko Sakkinen wrote:
> > Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
>
> Doesn't tpm_chip_register() need to be called by all drivers? This 
> function then calls tpm_chip_bootstrap().
>
> > cause a null derefence in tpm_buf_hmac_session*().  Thus, address
> > !chip->auth in tpm_buf_hmac_session*() and remove the fallback
> > implementation for !TCG_TPM2_HMAC.
> > 
> > Cc: stable@vger.kernel.org # v6.9+
> > Reported-by: Stefan Berger <stefanb@linux.ibm.com>
> > Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
> > Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> I applied this series now but it doesn't solve the reported problem. The 

It fixes the issues of which symptoms was shown by your transcript:

[    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
[    2.987140] ima: Error Communicating to TPM chip, result: -14

Your original thread identified zero problems, so thus your claim here
is plain untrue.

Before the null derefence is fixed all other patches related are
blocked, including ibm_tpmvtpm patches, because it would be insane
to accept them when there is known memory corruption bug, which
this patch set fixes.

What is so difficult to understand in this?

> error message is gone but the feature can still be enabled 
> (CONFIG_TCG_TPM2_HMAC=y) but is unlikely actually doing what it is 
> promising to do with this config option. So you either still have to 
> apply my patch, James's patch, or your intended "depends on 
> !TCG_IBMVTPM" patch.

Well this somewhat misleading imho...

None of the previous patches, including your, do nothing to fix the null
derefence bug and that is the *only* bug we care about ATM. With these
fixes drivers that do not call tpm_chip_bootstrap() will be fully
working still but without encryption.

There's five drivers which would require update for that:

drivers/char/tpm/tpm_ftpm_tee.c:        pvt_data->chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_i2c_nuvoton.c:             chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_ibmvtpm.c:         chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_tis_i2c_cr50.c:    chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_vtpm_proxy.c:              proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;


BR, Jarkko

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Jarkko Sakkinen]

On Thu Jul 4, 2024 at 4:56 AM EEST, Stefan Berger wrote:
> [    1.449673] tpm_ibmvtpm 5000: CRQ initialized
> [    1.449726] tpm_ibmvtpm 5000: CRQ initialization completed
> [    2.483218] tpm tpm0: auth session is not active

This expected result and the driver should work as expected.
And it correctly reports that auth session is not active.

The reported error was exactly the TPM errors, which I guess
do not happen anymore.

Reported-by counts as much as reporting the symptom you are
having, i.e. very different from something more "suggestive".

Does that count as tested-by or not?

BR, Jarkko

Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 11:11 PM EEST, James Bottomley wrote:
> On Wed, 2024-07-03 at 21:24 +0300, Jarkko Sakkinen wrote:
> [...]
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 21a67dc9efe8..2844fea4a12a 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -211,8 +211,8 @@ struct tpm_chip {
> >         u8 null_key_name[TPM2_NAME_SIZE];
> >         u8 null_ec_key_x[EC_PT_SZ];
> >         u8 null_ec_key_y[EC_PT_SZ];
> > -       struct tpm2_auth *auth;
> >  #endif
> > +       struct tpm2_auth *auth;
> >  };
>
> Since auth should only be present if CONFIG_TCG_TPM2_HMAC this is
> clearly an undesirable thing to do.  I think you did it because in a
> later patch you want to collapse the hmac sessions to use a single
> routine, but you can make that check with the preprocessor __and
> function defined in kconfig.h:
>
> if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
>
> Which will become 0 if the config is not enabled and chip->auth if it
> is, thus eliminating the code in the former case while not causing the
> compiler to complain about chip->auth not being defined even if it's
> under the config parameter.

I did not know about '__and()'. Thanks I'll use this!

>
> James

BR, Jarkko

Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[Linus Torvalds]

On Wed, 3 Jul 2024 at 13:11, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))

Augh. Please don't do this.

That "__and()" thing may work, but it's entirely accidental that it does.

It's designed for config options _only_, and the fact that it then
happens to work for "first argument is config option, second argument
is C conditional".

The comment says that it's implementing "&&" using preprocessor
expansion only, but it's a *really* limited form of it. The arguments
are *not* arbitrary.

So no. Don't do this.

Just create a helper inline like

    static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
    {
    #ifdef CONFIG_TCG_TPM2_HMAC
        return chip->auth;
    #else
        return NULL;
    #endif
    }

and if we really want to have some kind of automatic way of doing
this, we will *NOT* be using __and(), we'd do something like

        /* Return zero or 'value' depending on whether OPTION is
enabled or not */
        #define IF_ENABLED(option, value) __and(IS_ENABLED(option), value)

that actually would be documented and meaningful.

Not this internal random __and() implementation that is purely a
kconfig.h helper macro and SHOULD NOT be used anywhere else.

             Linus

Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[James Bottomley]

On Thu, 2024-07-04 at 10:07 -0700, Linus Torvalds wrote:
> On Wed, 3 Jul 2024 at 13:11, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > 
> > if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
> 
> Augh. Please don't do this.
> 
> That "__and()" thing may work, but it's entirely accidental that it
> does.
> 
> It's designed for config options _only_, and the fact that it then
> happens to work for "first argument is config option, second argument
> is C conditional".
> 
> The comment says that it's implementing "&&" using preprocessor
> expansion only, but it's a *really* limited form of it. The arguments
> are *not* arbitrary.
> 
> So no. Don't do this.
> 
> Just create a helper inline like
> 
>     static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
>     {
>     #ifdef CONFIG_TCG_TPM2_HMAC
>         return chip->auth;
>     #else
>         return NULL;
>     #endif
>     }
> 
> and if we really want to have some kind of automatic way of doing
> this, we will *NOT* be using __and(), we'd do something like
> 
>         /* Return zero or 'value' depending on whether OPTION is
> enabled or not */
>         #define IF_ENABLED(option, value) __and(IS_ENABLED(option),
> value)
> 
> that actually would be documented and meaningful.
> 
> Not this internal random __and() implementation that is purely a
> kconfig.h helper macro and SHOULD NOT be used anywhere else.

I actually like the latter version, but instinct tells me that if this
is the first time the kernel has ever needed something like this then
perhaps we should go with the former because that's how everyone must
have handled it in the past.

James

Re: [PATCH v2 2/3] tpm: Address !chip->auth in tpm_buf_append_name()

[Jarkko Sakkinen]

On Thu Jul 4, 2024 at 8:21 PM EEST, James Bottomley wrote:
> On Thu, 2024-07-04 at 10:07 -0700, Linus Torvalds wrote:
> > On Wed, 3 Jul 2024 at 13:11, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > 
> > > if (__and(IS_ENABLED(CONFIG_TCG_TPM2_HMAC), chip->auth))
> > 
> > Augh. Please don't do this.
> > 
> > That "__and()" thing may work, but it's entirely accidental that it
> > does.
> > 
> > It's designed for config options _only_, and the fact that it then
> > happens to work for "first argument is config option, second argument
> > is C conditional".
> > 
> > The comment says that it's implementing "&&" using preprocessor
> > expansion only, but it's a *really* limited form of it. The arguments
> > are *not* arbitrary.
> > 
> > So no. Don't do this.
> > 
> > Just create a helper inline like
> > 
> >     static inline struct tpm2_auth *chip_auth(struct tpm_chip *chip)
> >     {
> >     #ifdef CONFIG_TCG_TPM2_HMAC
> >         return chip->auth;
> >     #else
> >         return NULL;
> >     #endif
> >     }
> > 
> > and if we really want to have some kind of automatic way of doing
> > this, we will *NOT* be using __and(), we'd do something like
> > 
> >         /* Return zero or 'value' depending on whether OPTION is
> > enabled or not */
> >         #define IF_ENABLED(option, value) __and(IS_ENABLED(option),
> > value)
> > 
> > that actually would be documented and meaningful.
> > 
> > Not this internal random __and() implementation that is purely a
> > kconfig.h helper macro and SHOULD NOT be used anywhere else.
>
> I actually like the latter version, but instinct tells me that if this
> is the first time the kernel has ever needed something like this then
> perhaps we should go with the former because that's how everyone must
> have handled it in the past.

I'll go with the former given it is somewhat idiomatic and familiar
pattern.

> James

BR, Jarkko

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Stefan Berger]

On 7/4/24 02:41, Jarkko Sakkinen wrote:
> On Thu Jul 4, 2024 at 4:56 AM EEST, Stefan Berger wrote:
>>
>>
>> On 7/3/24 14:24, Jarkko Sakkinen wrote:
>>> Unless tpm_chip_bootstrap() was called by the driver, !chip->auth can
>>
>> Doesn't tpm_chip_register() need to be called by all drivers? This
>> function then calls tpm_chip_bootstrap().
>>
>>> cause a null derefence in tpm_buf_hmac_session*().  Thus, address
>>> !chip->auth in tpm_buf_hmac_session*() and remove the fallback
>>> implementation for !TCG_TPM2_HMAC.
>>>
>>> Cc: stable@vger.kernel.org # v6.9+
>>> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
>>> Closes: https://lore.kernel.org/linux-integrity/20240617193408.1234365-1-stefanb@linux.ibm.com/
>>> Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> I applied this series now but it doesn't solve the reported problem. The
> 
> It fixes the issues of which symptoms was shown by your transcript:
> 
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
> 
> Your original thread identified zero problems, so thus your claim here
> is plain untrue.

The original thread here

https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t

identified the fact that tpm2_session_init() was missing for the ibmvtpm 
driver. It is a non-zero problem for the respective platforms where this 
driver is being used. The patched fixed the reported issue.

> 
> Before the null derefence is fixed all other patches related are
> blocked, including ibm_tpmvtpm patches, because it would be insane
> to accept them when there is known memory corruption bug, which
> this patch set fixes.
> 
> What is so difficult to understand in this?
> 
>> error message is gone but the feature can still be enabled
>> (CONFIG_TCG_TPM2_HMAC=y) but is unlikely actually doing what it is
>> promising to do with this config option. So you either still have to
>> apply my patch, James's patch, or your intended "depends on
>> !TCG_IBMVTPM" patch.
> 
> Well this somewhat misleading imho...
> 
> None of the previous patches, including your, do nothing to fix the null
> derefence bug and that is the *only* bug we care about ATM. With these
> fixes drivers that do not call tpm_chip_bootstrap() will be fully
> working still but without encryption.
> 

Now that you fixed it in v4 are you going to accept my original patch 
with the Fixes tag since we will (likely) have an enabled feature in 
6.10 that is not actually working when the ibmvtpm driver is being used?

Original patch:

https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t

> There's five drivers which would require update for that:
> 
> drivers/char/tpm/tpm_ftpm_tee.c:        pvt_data->chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_i2c_nuvoton.c:             chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_ibmvtpm.c:         chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_tis_i2c_cr50.c:    chip->flags |= TPM_CHIP_FLAG_TPM2;
> drivers/char/tpm/tpm_vtpm_proxy.c:              proxy_dev->chip->flags |= TPM_CHIP_FLAG_TPM2;

I do no think that this is true and its only tpm_ibmvtpm.c that need the 
call to tpm2_session_init. All drivers that use TPM_OPS_AUTO_STARTUP 
will run tpm_chip_register -> tpm_chip_bootstrap -> tpm_auto_startup -> 
tpm2_auto_startup -> tpm2_sessions_init

$ grep AUTO_START *.c
tpm_crb.c:      .flags = TPM_OPS_AUTO_STARTUP,
tpm_ftpm_tee.c: .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_atmel.c:        .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_infineon.c:     .flags = TPM_OPS_AUTO_STARTUP,
tpm_i2c_nuvoton.c:      .flags = TPM_OPS_AUTO_STARTUP,
tpm-interface.c:        if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
tpm_tis_core.c: .flags = TPM_OPS_AUTO_STARTUP,
tpm_tis_i2c_cr50.c:     .flags = TPM_OPS_AUTO_STARTUP,
tpm_vtpm_proxy.c:       .flags = TPM_OPS_AUTO_STARTUP,

All the above drivers are also calling tpm_chip_register.

tpm_atmel.c:    rc = tpm_chip_register(chip);
tpm-chip.c: * tpm_chip_register() - create a character device for the 
TPM chip
tpm-chip.c:int tpm_chip_register(struct tpm_chip *chip)
tpm-chip.c:EXPORT_SYMBOL_GPL(tpm_chip_register);
tpm-chip.c: * cleans up all the resources reserved by tpm_chip_register().
tpm_crb.c:      rc = tpm_chip_register(chip);
tpm_ftpm_tee.c: rc = tpm_chip_register(pvt_data->chip);
tpm_ftpm_tee.c:         dev_err(dev, "%s: tpm_chip_register failed with 
rc=%d\n",
tpm_i2c_atmel.c:        return tpm_chip_register(chip);
tpm_i2c_infineon.c:     return tpm_chip_register(chip);
tpm_i2c_nuvoton.c:      return tpm_chip_register(chip);
tpm_ibmvtpm.c:  return tpm_chip_register(chip);
tpm_infineon.c:         rc = tpm_chip_register(chip);
tpm_nsc.c:      rc = tpm_chip_register(chip);
tpm_tis_core.c: rc = tpm_chip_register(chip);
tpm_tis_i2c_cr50.c:     return tpm_chip_register(chip);
tpm_vtpm_proxy.c:       rc = tpm_chip_register(proxy_dev->chip);
xen-tpmfront.c: return tpm_chip_register(priv->chip)


   Stefan

> 
> 
> BR, Jarkko

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Jarkko Sakkinen]

On Fri Jul 5, 2024 at 5:05 PM EEST, Stefan Berger wrote:
> The original thread here
>
> https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t
>
> identified the fact that tpm2_session_init() was missing for the ibmvtpm 
> driver. It is a non-zero problem for the respective platforms where this 
> driver is being used. The patched fixed the reported issue.

All bugs needs to be fixed always before features are added. You are
free now to submit your change as a feature patch, which will be
reviewed and applied later on.

> Now that you fixed it in v4 are you going to accept my original patch 
> with the Fixes tag since we will (likely) have an enabled feature in 
> 6.10 that is not actually working when the ibmvtpm driver is being used?

There's no bug in tpm_ibmvtpm driver as it functions as well as in 6.9.

I can review it earliest in the week 31, as feature patch. This was my
holiday week, and I came back only to fix the bug in the authentication
session patch set.

> I do no think that this is true and its only tpm_ibmvtpm.c that need the 
> call to tpm2_session_init. All drivers that use TPM_OPS_AUTO_STARTUP 
> will run tpm_chip_register -> tpm_chip_bootstrap -> tpm_auto_startup -> 
> tpm2_auto_startup -> tpm2_sessions_init

Right my bad. I overlooked the call sites and you're correct in that
for anything with that flag on, it will be called.

It still changes nothing, as the commit you were pointing out in the
fixes tag does not implement initialization code, and we would not have
that flag in the first place, if it was mandatory [1].

[1] It could be that it is mandatory perhaps, but that is a different
story. Then we would render the whole flag out. I think this was anyway
good insight, even if by unintentionally, and we can reconsider removing
it some day.

BR, Jarkko

Re: [PATCH v2 3/3] tpm: Address !chip->auth in tpm_buf_append_hmac_session*()

[Jarkko Sakkinen]

On Fri Jul 5, 2024 at 5:35 PM EEST, Jarkko Sakkinen wrote:
> On Fri Jul 5, 2024 at 5:05 PM EEST, Stefan Berger wrote:
> > The original thread here
> >
> > https://lore.kernel.org/linux-integrity/656b319fc58683e399323b880722434467cf20f2.camel@kernel.org/T/#t
> >
> > identified the fact that tpm2_session_init() was missing for the ibmvtpm 
> > driver. It is a non-zero problem for the respective platforms where this 
> > driver is being used. The patched fixed the reported issue.
>
> All bugs needs to be fixed always before features are added. You are
> free now to submit your change as a feature patch, which will be
> reviewed and applied later on.
>
> > Now that you fixed it in v4 are you going to accept my original patch 
> > with the Fixes tag since we will (likely) have an enabled feature in 
> > 6.10 that is not actually working when the ibmvtpm driver is being used?
>
> There's no bug in tpm_ibmvtpm driver as it functions as well as in 6.9.
>
> I can review it earliest in the week 31, as feature patch. This was my
> holiday week, and I came back only to fix the bug in the authentication
> session patch set.
>
> > I do no think that this is true and its only tpm_ibmvtpm.c that need the 
> > call to tpm2_session_init. All drivers that use TPM_OPS_AUTO_STARTUP 
> > will run tpm_chip_register -> tpm_chip_bootstrap -> tpm_auto_startup -> 
> > tpm2_auto_startup -> tpm2_sessions_init
>
> Right my bad. I overlooked the call sites and you're correct in that
> for anything with that flag on, it will be called.
>
> It still changes nothing, as the commit you were pointing out in the
> fixes tag does not implement initialization code, and we would not have
> that flag in the first place, if it was mandatory [1].
>
> [1] It could be that it is mandatory perhaps, but that is a different
> story. Then we would render the whole flag out. I think this was anyway
> good insight, even if by unintentionally, and we can reconsider removing
> it some day.

I should have rejected the patch set based on not validating chip->auth
in opaque API that tpm2-sessions is, and it should not fail caller like
that no matter how world outside of it is structured. It's a time-bomb
like it is in the mainline because of this.  I missed that detail
and your transcript exposed the bug.

Working around an *identified* bug in the caller *is not* a bug fix.

BR, Jarkko