[PATCH] tpm2_load_command leaks memory

Subsystems: keys-trusted, keys/keyrings, security subsystem, the rest

STALE1889d

3 messages, 3 authors, 2021-06-16 · open the first message on its own page

[PATCH] tpm2_load_command leaks memory

From: Dhiraj Shah <hidden>
Date: 2021-06-10 09:50:27

tpm2_key_decode allocates memory which is stored in blob and it's not freed.

Signed-off-by: Dhiraj Shah <redacted>
---
 security/keys/trusted-keys/trusted_tpm2.c | 41 +++++++++++++++--------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..52dd43bb8cdb 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c

@@ -378,22 +378,31 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	}
 
 	/* new format carries keyhandle but old format doesn't */
-	if (!options->keyhandle)
-		return -EINVAL;
+	if (!options->keyhandle) {
+		rc = -EINVAL;
+		goto err;
+	}
 
 	/* must be big enough for at least the two be16 size counts */
-	if (payload->blob_len < 4)
-		return -EINVAL;
+	if (payload->blob_len < 4) {
+		rc = -EINVAL;
+		goto err;
+	}
 
 	private_len = get_unaligned_be16(blob);
 
 	/* must be big enough for following public_len */
-	if (private_len + 2 + 2 > (payload->blob_len))
-		return -E2BIG;
+	if (private_len + 2 + 2 > (payload->blob_len)) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	public_len = get_unaligned_be16(blob + 2 + private_len);
-	if (private_len + 2 + public_len + 2 > payload->blob_len)
-		return -E2BIG;
+
+	if (private_len + 2 + public_len + 2 > payload->blob_len) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	pub = blob + 2 + private_len + 2;
 	/* key attributes are always at offset 4 */

@@ -406,13 +415,16 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		payload->migratable = 1;
 
 	blob_len = private_len + public_len + 4;
-	if (blob_len > payload->blob_len)
-		return -E2BIG;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
-	if (rc)
-		return rc;
+	if (blob_len > payload->blob_len) {
+		rc = -E2BIG;
+		goto err;
+	}
 
+	if (tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD) != 0)
+		rc = -ENOMEM;
+		goto out;
+	}
 	tpm_buf_append_u32(&buf, options->keyhandle);
 	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
 			     NULL /* nonce */, 0,

@@ -433,9 +445,10 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
 
 out:
+	tpm_buf_destroy(&buf);
+err:
 	if (blob != payload->blob)
 		kfree(blob);
-	tpm_buf_destroy(&buf);
 
 	if (rc > 0)
 		rc = -EPERM;

-- 
2.30.1 (Apple Git-130)

Re: [PATCH] tpm2_load_command leaks memory

From: kernel test robot <hidden>
Date: 2021-06-16 14:40:55

Hi Dhiraj,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v5.13-rc6]
[also build test ERROR on next-20210616]
[cannot apply to security/next-testing]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dhiraj-Shah/tpm2_load_command-leaks-memory/20210616-184020
base:    009c9aa5be652675a06d5211e1640e02bbb1c33d
config: s390-randconfig-r014-20210615 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 64720f57bea6a6bf033feef4a5751ab9c0c3b401)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/985c6fcde5d80fed97392f94b906e6b43c164f47
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dhiraj-Shah/tpm2_load_command-leaks-memory/20210616-184020
        git checkout 985c6fcde5d80fed97392f94b906e6b43c164f47
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All error/warnings (new ones prefixed by >>):

quoted

security/keys/trusted-keys/trusted_tpm2.c:426:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation]

                   goto out;
                   ^
   security/keys/trusted-keys/trusted_tpm2.c:424:2: note: previous statement is here
           if (tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD) != 0)
           ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:426:8: error: use of undeclared label 'out'

                   goto out;
                        ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:383:8: error: use of undeclared label 'err'

                   goto err;
                        ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:428:21: error: expected parameter declarator

           tpm_buf_append_u32(&buf, options->keyhandle);
                              ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:428:21: error: expected ')'

   security/keys/trusted-keys/trusted_tpm2.c:428:20: note: to match this '('
           tpm_buf_append_u32(&buf, options->keyhandle);
                             ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:428:2: warning: declaration specifier missing, defaulting to 'int'

           tpm_buf_append_u32(&buf, options->keyhandle);
           ^
           int

quoted

security/keys/trusted-keys/trusted_tpm2.c:428:20: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]

           tpm_buf_append_u32(&buf, options->keyhandle);
                             ^
                                                      void

quoted

security/keys/trusted-keys/trusted_tpm2.c:428:2: error: conflicting types for 'tpm_buf_append_u32'

           tpm_buf_append_u32(&buf, options->keyhandle);
           ^
   include/linux/tpm.h:394:20: note: previous definition is here
   static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
                      ^
   security/keys/trusted-keys/trusted_tpm2.c:429:23: error: expected parameter declarator
           tpm2_buf_append_auth(&buf, TPM2_RS_PW,
                                ^
   security/keys/trusted-keys/trusted_tpm2.c:429:23: error: expected ')'
   security/keys/trusted-keys/trusted_tpm2.c:429:22: note: to match this '('
           tpm2_buf_append_auth(&buf, TPM2_RS_PW,
                               ^
   security/keys/trusted-keys/trusted_tpm2.c:429:2: warning: declaration specifier missing, defaulting to 'int'
           tpm2_buf_append_auth(&buf, TPM2_RS_PW,
           ^
           int
   security/keys/trusted-keys/trusted_tpm2.c:429:22: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
           tpm2_buf_append_auth(&buf, TPM2_RS_PW,
                               ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:429:2: error: conflicting types for 'tpm2_buf_append_auth'

           tpm2_buf_append_auth(&buf, TPM2_RS_PW,
           ^
   security/keys/trusted-keys/trusted_tpm2.c:199:13: note: previous definition is here
   static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
               ^
   security/keys/trusted-keys/trusted_tpm2.c:435:17: error: expected parameter declarator
           tpm_buf_append(&buf, blob, blob_len);
                          ^
   security/keys/trusted-keys/trusted_tpm2.c:435:17: error: expected ')'
   security/keys/trusted-keys/trusted_tpm2.c:435:16: note: to match this '('
           tpm_buf_append(&buf, blob, blob_len);
                         ^
   security/keys/trusted-keys/trusted_tpm2.c:435:2: warning: declaration specifier missing, defaulting to 'int'
           tpm_buf_append(&buf, blob, blob_len);
           ^
           int
   security/keys/trusted-keys/trusted_tpm2.c:435:16: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
           tpm_buf_append(&buf, blob, blob_len);
                         ^
                                              void

quoted

security/keys/trusted-keys/trusted_tpm2.c:435:2: error: conflicting types for 'tpm_buf_append'

           tpm_buf_append(&buf, blob, blob_len);
           ^
   include/linux/tpm.h:361:20: note: previous definition is here
   static inline void tpm_buf_append(struct tpm_buf *buf,
                      ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:437:2: error: expected identifier or '('

           if (buf.flags & TPM_BUF_OVERFLOW) {
           ^
   security/keys/trusted-keys/trusted_tpm2.c:442:2: warning: declaration specifier missing, defaulting to 'int'
           rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
           ^
           int

quoted

security/keys/trusted-keys/trusted_tpm2.c:442:24: error: use of undeclared identifier 'chip'

           rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
                                 ^

quoted

security/keys/trusted-keys/trusted_tpm2.c:442:31: error: use of undeclared identifier 'buf'

           rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
                                        ^
   security/keys/trusted-keys/trusted_tpm2.c:443:2: error: expected identifier or '('
           if (!rc)
           ^
   security/keys/trusted-keys/trusted_tpm2.c:447:1: warning: declaration specifier missing, defaulting to 'int'
   out:
   ^
   int

quoted

security/keys/trusted-keys/trusted_tpm2.c:447:4: error: expected ';' after top level declarator

   out:
      ^
      ;
   fatal error: too many errors emitted, stopping now [-ferror-limit=]
   6 warnings and 20 errors generated.


vim +/out +426 security/keys/trusted-keys/trusted_tpm2.c

   346	
   347	/**
   348	 * tpm2_load_cmd() - execute a TPM2_Load command
   349	 *
   350	 * @chip: TPM chip to use
   351	 * @payload: the key data in clear and encrypted form
   352	 * @options: authentication values and other options
   353	 * @blob_handle: returned blob handle
   354	 *
   355	 * Return: 0 on success.
   356	 *        -E2BIG on wrong payload size.
   357	 *        -EPERM on tpm error status.
   358	 *        < 0 error from tpm_send.
   359	 */
   360	static int tpm2_load_cmd(struct tpm_chip *chip,
   361				 struct trusted_key_payload *payload,
   362				 struct trusted_key_options *options,
   363				 u32 *blob_handle)
   364	{
   365		struct tpm_buf buf;
   366		unsigned int private_len;
   367		unsigned int public_len;
   368		unsigned int blob_len;
   369		u8 *blob, *pub;
   370		int rc;
   371		u32 attrs;
   372	
   373		rc = tpm2_key_decode(payload, options, &blob);
   374		if (rc) {
   375			/* old form */
   376			blob = payload->blob;
   377			payload->old_format = 1;
   378		}
   379	
   380		/* new format carries keyhandle but old format doesn't */
   381		if (!options->keyhandle) {
   382			rc = -EINVAL;
 > 383			goto err;
   384		}
   385	
   386		/* must be big enough for at least the two be16 size counts */
   387		if (payload->blob_len < 4) {
   388			rc = -EINVAL;
   389			goto err;
   390		}
   391	
   392		private_len = get_unaligned_be16(blob);
   393	
   394		/* must be big enough for following public_len */
   395		if (private_len + 2 + 2 > (payload->blob_len)) {
   396			rc = -E2BIG;
   397			goto err;
   398		}
   399	
   400		public_len = get_unaligned_be16(blob + 2 + private_len);
   401	
   402		if (private_len + 2 + public_len + 2 > payload->blob_len) {
   403			rc = -E2BIG;
   404			goto err;
   405		}
   406	
   407		pub = blob + 2 + private_len + 2;
   408		/* key attributes are always at offset 4 */
   409		attrs = get_unaligned_be32(pub + 4);
   410	
   411		if ((attrs & (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT)) ==
   412		    (TPM2_OA_FIXED_TPM | TPM2_OA_FIXED_PARENT))
   413			payload->migratable = 0;
   414		else
   415			payload->migratable = 1;
   416	
   417		blob_len = private_len + public_len + 4;
   418	
   419		if (blob_len > payload->blob_len) {
   420			rc = -E2BIG;
   421			goto err;
   422		}
   423	
 > 424		if (tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD) != 0)
   425			rc = -ENOMEM;
 > 426			goto out;
   427		}
 > 428		tpm_buf_append_u32(&buf, options->keyhandle);
 > 429		tpm2_buf_append_auth(&buf, TPM2_RS_PW,
   430				     NULL /* nonce */, 0,
   431				     0 /* session_attributes */,
   432				     options->keyauth /* hmac */,
   433				     TPM_DIGEST_SIZE);
   434	
 > 435		tpm_buf_append(&buf, blob, blob_len);
   436	
 > 437		if (buf.flags & TPM_BUF_OVERFLOW) {
   438			rc = -E2BIG;
   439			goto out;
   440		}
   441	
 > 442		rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
 > 443		if (!rc)
   444			*blob_handle = be32_to_cpup(
   445				(__be32 *) &buf.data[TPM_HEADER_SIZE]);
   446	
 > 447	out:
   448		tpm_buf_destroy(&buf);
   449	err:
   450		if (blob != payload->blob)
   451			kfree(blob);
   452	
   453		if (rc > 0)
   454			rc = -EPERM;
   455	
   456		return rc;
   457	}
   458	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH] tpm2_load_command leaks memory

From: James Bottomley <hidden>
Date: 2021-06-16 14:50:18

On Thu, 2021-06-10 at 10:49 +0100, Dhiraj Shah wrote:

quoted hunk

tpm2_key_decode allocates memory which is stored in blob and it's not
freed.

Signed-off-by: Dhiraj Shah <redacted>
---
 security/keys/trusted-keys/trusted_tpm2.c | 41 +++++++++++++++----
----
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c

b/security/keys/trusted-keys/trusted_tpm2.c
index 0165da386289..52dd43bb8cdb 100644

--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c

@@ -378,22 +378,31 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 	}
 
 	/* new format carries keyhandle but old format doesn't */
-	if (!options->keyhandle)
-		return -EINVAL;
+	if (!options->keyhandle) {
+		rc = -EINVAL;
+		goto err;
+	}

This one is unnecessary ... for the old format there's nothing to free.

quoted hunk

 	/* must be big enough for at least the two be16 size counts */
-	if (payload->blob_len < 4)
-		return -EINVAL;
+	if (payload->blob_len < 4) {
+		rc = -EINVAL;
+		goto err;
+	}
 
 	private_len = get_unaligned_be16(blob);
 
 	/* must be big enough for following public_len */
-	if (private_len + 2 + 2 > (payload->blob_len))
-		return -E2BIG;
+	if (private_len + 2 + 2 > (payload->blob_len)) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	public_len = get_unaligned_be16(blob + 2 + private_len);
-	if (private_len + 2 + public_len + 2 > payload->blob_len)
-		return -E2BIG;
+
+	if (private_len + 2 + public_len + 2 > payload->blob_len) {
+		rc = -E2BIG;
+		goto err;
+	}
 
 	pub = blob + 2 + private_len + 2;
 	/* key attributes are always at offset 4 */

@@ -406,13 +415,16 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
 		payload->migratable = 1;
 
 	blob_len = private_len + public_len + 4;
-	if (blob_len > payload->blob_len)
-		return -E2BIG;
 
-	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
-	if (rc)
-		return rc;
+	if (blob_len > payload->blob_len) {
+		rc = -E2BIG;
+		goto err;
+	}
 
+	if (tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD) != 0)

You didn't compile this, did you?  There's no opening brace here ...

James

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help