Message-ID: <aS9jOs0872QL__l3@kernel.org>
Date: Wed, 3 Dec 2025 00:07:54 +0200
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Jonathan McDowell <noodles@...th.li>
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 Tue, Dec 02, 2025 at 09:53:39PM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 02, 2025 at 04:26:09PM +0000, Jonathan McDowell wrote:
> > 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?
> 
> I think it is a good idea to do that right now.
> 
> Also, it'd be better to not have SM3 label as SM2/SM3 is not supported
> at this point. I'm working simulatenously on an improved feature and
> that slipped from that work:
> 
> My big picture roadmap for this feature, and how to make it useful  is:
> 
> 1. tpm.integrity_mode=disabled/permissive/enforced. Permissive means
> here that the feature is conditionally enabled if algorithms that are
> required to enable the HMAC pipe are available.
> 2. tpm.integrity_handle=0x00000000/0x81??????.
> 
> > 
> > > +	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?
> 
> Thanks, I'll add it back.
> 
> > 
> > > +		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.
> 
> Agreed.
> 
> > 
> > > -		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
> 
> BR, Jarkko

Addressed in: https://lore.kernel.org/linux-integrity/20251202202643.107108-2-jarkko@kernel.org/

BR, Jarkko

Powered by blists - more mailing lists