| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aS8TIeviaippVAha@earth.li>
Date: Tue, 2 Dec 2025 16:26:09 +0000
From: Jonathan McDowell <noodles@...th.li>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: linux-integrity@...r.kernel.org,
Stefano Garzarella <sgarzare@...hat.com>, stable@...r.kernel.org,
Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
James Bottomley <James.Bottomley@...senpartnership.com>,
Mimi Zohar <zohar@...ux.ibm.com>,
David Howells <dhowells@...hat.com>,
Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
Ard Biesheuvel <ardb@...nel.org>, linux-kernel@...r.kernel.org,
keyrings@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v3] tpm2-sessions: address out-of-range indexing
On Mon, Dec 01, 2025 at 09:39:58PM +0200, Jarkko Sakkinen wrote:
>'name_size' does not have any range checks, and it just directly indexes
>with TPM_ALG_ID, which could lead into memory corruption at worst.
>
>Address the issue by only processing known values and returning -EINVAL for
>unrecognized values.
>
>Make also 'tpm_buf_append_name' and 'tpm_buf_fill_hmac_session' fallible so
>that errors are detected before causing any spurious TPM traffic.
>
>End also the authorization session on failure in both of the functions, as
>the session state would be then by definition corrupted.
>
>Cc: stable@...r.kernel.org # v6.10+
>Fixes: 1085b8276bb4 ("tpm: Add the rest of the session HMAC API")
>Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
>---
>v3:
>- Add two missing 'tpm2_end_auth_session' calls to the fallback paths of
> 'tpm_buf_fill_hmac_session'.
>- Rewrote the commit message.
>- End authorization session on failure in 'tpm2_buf_append_name' and
> 'tpm_buf_fill_hmac_session'.
>v2:
>There was spurious extra field added to tpm2_hash by mistake.
>---
> drivers/char/tpm/tpm2-cmd.c | 23 +++-
> drivers/char/tpm/tpm2-sessions.c | 131 +++++++++++++++-------
> include/linux/tpm.h | 6 +-
> security/keys/trusted-keys/trusted_tpm2.c | 29 ++++-
> 4 files changed, 136 insertions(+), 53 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>index 5b6ccf901623..4473b81122e8 100644
>--- a/drivers/char/tpm/tpm2-cmd.c
>+++ b/drivers/char/tpm/tpm2-cmd.c
>@@ -187,7 +187,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> }
>
> if (!disable_pcr_integrity) {
>- tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
>+ if (rc) {
>+ tpm_buf_destroy(&buf);
>+ return rc;
>+ }
> tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> } else {
> tpm_buf_append_handle(chip, &buf, pcr_idx);
>@@ -202,8 +206,14 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> chip->allocated_banks[i].digest_size);
> }
>
>- if (!disable_pcr_integrity)
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ if (!disable_pcr_integrity) {
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc) {
>+ tpm_buf_destroy(&buf);
>+ return rc;
>+ }
>+ }
>+
> rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
> if (!disable_pcr_integrity)
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>@@ -261,7 +271,12 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
> | TPM2_SA_CONTINUE_SESSION,
> NULL, 0);
> tpm_buf_append_u16(&buf, num_bytes);
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ err = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (err) {
>+ tpm_buf_destroy(&buf);
>+ return err;
>+ }
>+
> err = tpm_transmit_cmd(chip, &buf,
> offsetof(struct tpm2_get_random_out,
> buffer),
>diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
>index 6d03c224e6b2..33ad0d668e1a 100644
>--- a/drivers/char/tpm/tpm2-sessions.c
>+++ b/drivers/char/tpm/tpm2-sessions.c
>@@ -144,16 +144,24 @@ struct tpm2_auth {
> /*
> * Name Size based on TPM algorithm (assumes no hash bigger than 255)
> */
>-static u8 name_size(const u8 *name)
>+static int name_size(const u8 *name)
> {
>- static u8 size_map[] = {
>- [TPM_ALG_SHA1] = SHA1_DIGEST_SIZE,
>- [TPM_ALG_SHA256] = SHA256_DIGEST_SIZE,
>- [TPM_ALG_SHA384] = SHA384_DIGEST_SIZE,
>- [TPM_ALG_SHA512] = SHA512_DIGEST_SIZE,
>- };
>- u16 alg = get_unaligned_be16(name);
>- return size_map[alg] + 2;
>+ u16 hash_alg = get_unaligned_be16(name);
>+
>+ switch (hash_alg) {
>+ case TPM_ALG_SHA1:
>+ return SHA1_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA256:
>+ return SHA256_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA384:
>+ return SHA384_DIGEST_SIZE + 2;
>+ case TPM_ALG_SHA512:
>+ return SHA512_DIGEST_SIZE + 2;
>+ case TPM_ALG_SM3_256:
>+ return SM3256_DIGEST_SIZE + 2;
>+ }
>+
Can we/should we perhaps print a warning here if we don't know the
algorithm?
>+ return -EINVAL;
> }
>
> static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
>@@ -161,6 +169,7 @@ 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);
>+ int ret;
> u32 val;
>
> /* we're starting after the header so adjust the length */
>@@ -172,9 +181,15 @@ static int tpm2_parse_read_public(char *name, struct tpm_buf *buf)
> return -EINVAL;
> offset += val;
> /* name */
>+
> val = tpm_buf_read_u16(buf, &offset);
>- if (val != name_size(&buf->data[offset]))
>+ ret = name_size(&buf->data[offset]);
>+ if (ret < 0)
>+ return ret;
>+
>+ if (val != ret)
> return -EINVAL;
>+
> memcpy(name, &buf->data[offset], val);
> /* forget the rest */
> return 0;
>@@ -221,46 +236,70 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *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.
>+ *
>+ * Ends the authorization session on failure.
> */
>-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>- u32 handle, u8 *name)
>+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>+ u32 handle, u8 *name)
> {
> #ifdef CONFIG_TCG_TPM2_HMAC
> enum tpm2_mso_type mso = tpm2_handle_mso(handle);
> struct tpm2_auth *auth;
> int slot;
>+ int ret;
> #endif
>
> if (!tpm2_chip_auth(chip)) {
> tpm_buf_append_handle(chip, buf, handle);
>- return;
>+ return 0;
> }
>
> #ifdef CONFIG_TCG_TPM2_HMAC
> slot = (tpm_buf_length(buf) - TPM_HEADER_SIZE) / 4;
> if (slot >= AUTH_MAX_NAMES) {
>- dev_err(&chip->dev, "TPM: too many handles\n");
>- return;
>+ dev_err(&chip->dev, "too many handles\n");
>+ ret = -EIO;
>+ goto err;
> }
> auth = chip->auth;
>- WARN(auth->session != tpm_buf_length(buf),
>- "name added in wrong place\n");
>+ if (auth->session != tpm_buf_length(buf)) {
>+ dev_err(&chip->dev, "session state malformed");
>+ ret = -EIO;
>+ goto err;
>+ }
> 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]);
>+ if (!name) {
>+ ret = tpm2_read_public(chip, handle, auth->name[slot]);
>+ if (ret)
>+ goto err;
>+ }
> } else {
>- if (name)
>- dev_err(&chip->dev, "TPM: Handle does not require name but one is specified\n");
We're dropping the error message here; is there a reason for that?
>+ if (name) {
>+ ret = -EIO;
>+ goto err;
>+ }
> }
>
> auth->name_h[slot] = handle;
>- if (name)
>- memcpy(auth->name[slot], name, name_size(name));
>+ if (name) {
>+ ret = name_size(name);
>+ if (ret < 0)
>+ goto err;
>+
>+ memcpy(auth->name[slot], name, ret);
>+ }
>+#endif
>+ return 0;
>+
>+#ifdef CONFIG_TCG_TPM2_HMAC
>+err:
>+ tpm2_end_auth_session(chip);
>+ return tpm_ret_to_err(ret);
> #endif
> }
> EXPORT_SYMBOL_GPL(tpm_buf_append_name);
>@@ -533,11 +572,9 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> * encryption key and encrypts the first parameter of the command
> * buffer with it.
> *
>- * 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.
>+ * Ends the authorization session on failure.
> */
>-void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
>+int tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> {
> u32 cc, handles, val;
> struct tpm2_auth *auth = chip->auth;
>@@ -549,9 +586,12 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> u8 cphash[SHA256_DIGEST_SIZE];
> struct sha256_ctx sctx;
> struct hmac_sha256_ctx hctx;
>+ int ret;
>
>- if (!auth)
>- return;
>+ if (!auth) {
>+ ret = -EINVAL;
>+ goto err;
>+ }
>
> /* save the command code in BE format */
> auth->ordinal = head->ordinal;
>@@ -560,9 +600,10 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
>
> i = tpm2_find_cc(chip, cc);
> if (i < 0) {
>- dev_err(&chip->dev, "Command 0x%x not found in TPM\n", cc);
Again, I think it's generally helpful to have the error message given
that the return (EINVAL) does not help narrow down which value is bad.
>- return;
>+ ret = -EINVAL;
>+ goto err;
> }
>+
> attrs = chip->cc_attrs_tbl[i];
>
> handles = (attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0);
>@@ -576,9 +617,9 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> u32 handle = tpm_buf_read_u32(buf, &offset_s);
>
> if (auth->name_h[i] != handle) {
>- dev_err(&chip->dev, "TPM: handle %d wrong for name\n",
>- i);
>- return;
>+ dev_err(&chip->dev, "invalid handle 0x%08x\n", handle);
>+ ret = -EINVAL;
>+ goto err;
> }
> }
> /* point offset_s to the start of the sessions */
>@@ -609,12 +650,14 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> offset_s += len;
> }
> if (offset_s != offset_p) {
>- dev_err(&chip->dev, "TPM session length is incorrect\n");
>- return;
>+ dev_err(&chip->dev, "session length is incorrect\n");
>+ ret = -EINVAL;
>+ goto err;
> }
> if (!hmac) {
>- dev_err(&chip->dev, "TPM could not find HMAC session\n");
>- return;
>+ dev_err(&chip->dev, "could not find HMAC session\n");
>+ ret = -EINVAL;
>+ goto err;
> }
>
> /* encrypt before HMAC */
>@@ -646,8 +689,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> if (mso == TPM2_MSO_PERSISTENT ||
> mso == TPM2_MSO_VOLATILE ||
> mso == TPM2_MSO_NVRAM) {
>- sha256_update(&sctx, auth->name[i],
>- name_size(auth->name[i]));
>+ ret = name_size(auth->name[i]);
>+ if (ret < 0)
>+ goto err;
>+
>+ sha256_update(&sctx, auth->name[i], ret);
> } else {
> __be32 h = cpu_to_be32(auth->name_h[i]);
>
>@@ -668,6 +714,11 @@ void tpm_buf_fill_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf)
> hmac_sha256_update(&hctx, auth->tpm_nonce, sizeof(auth->tpm_nonce));
> hmac_sha256_update(&hctx, &auth->attrs, 1);
> hmac_sha256_final(&hctx, hmac);
>+ return 0;
>+
>+err:
>+ tpm2_end_auth_session(chip);
>+ return ret;
> }
> EXPORT_SYMBOL(tpm_buf_fill_hmac_session);
>
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index 0e9e043f728c..1a59f0190eb3 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -528,8 +528,8 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
> #endif
> }
>
>-void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>- u32 handle, u8 *name);
>+int tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
>+ u32 handle, u8 *name);
> void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> u8 attributes, u8 *passphrase,
> int passphraselen);
>@@ -562,7 +562,7 @@ static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> #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_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);
> void tpm2_end_auth_session(struct tpm_chip *chip);
>diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>index e165b117bbca..7672a4376dad 100644
>--- a/security/keys/trusted-keys/trusted_tpm2.c
>+++ b/security/keys/trusted-keys/trusted_tpm2.c
>@@ -283,7 +283,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out_put;
> }
>
>- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>+
> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_DECRYPT,
> options->keyauth, TPM_DIGEST_SIZE);
>
>@@ -331,7 +334,10 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out;
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> if (rc)
>@@ -438,7 +444,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> return rc;
> }
>
>- tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>+
> tpm_buf_append_hmac_session(chip, &buf, 0, options->keyauth,
> TPM_DIGEST_SIZE);
>
>@@ -450,7 +459,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> goto out;
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> if (!rc)
>@@ -497,7 +509,9 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> return rc;
> }
>
>- tpm_buf_append_name(chip, &buf, blob_handle, NULL);
>+ rc = tpm_buf_append_name(chip, &buf, options->keyhandle, NULL);
>+ if (rc)
>+ goto out;
>
> if (!options->policyhandle) {
> tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT,
>@@ -522,7 +536,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> NULL, 0);
> }
>
>- tpm_buf_fill_hmac_session(chip, &buf);
>+ rc = tpm_buf_fill_hmac_session(chip, &buf);
>+ if (rc)
>+ goto out;
>+
> rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>
>--
>2.52.0
>
J.
--
Be Ye Not Lost Among Precepts of Order
Powered by blists - more mailing lists