linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tpm2_load_command leaks memory
@ 2021-06-10  9:49 Dhiraj Shah
  2021-06-16 14:40 ` kernel test robot
  2021-06-16 14:49 ` James Bottomley
  0 siblings, 2 replies; 3+ messages in thread
From: Dhiraj Shah @ 2021-06-10  9:49 UTC (permalink / raw)
  Cc: find.dhiraj, James Bottomley, Jarkko Sakkinen, Mimi Zohar,
	David Howells, James Morris, Serge E. Hallyn, linux-integrity,
	keyrings, linux-security-module, linux-kernel

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

Signed-off-by: Dhiraj Shah <find.dhiraj@gmail.com>
---
 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)


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tpm2_load_command leaks memory
  2021-06-10  9:49 [PATCH] tpm2_load_command leaks memory Dhiraj Shah
@ 2021-06-16 14:40 ` kernel test robot
  2021-06-16 14:49 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2021-06-16 14:40 UTC (permalink / raw)
  To: Dhiraj Shah
  Cc: kbuild-all, clang-built-linux, find.dhiraj, James Bottomley,
	Jarkko Sakkinen, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, linux-integrity, keyrings,
	linux-security-module

[-- Attachment #1: Type: text/plain, Size: 10452 bytes --]

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 <lkp@intel.com>

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

>> 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)
           ^
>> security/keys/trusted-keys/trusted_tpm2.c:426:8: error: use of undeclared label 'out'
                   goto out;
                        ^
>> security/keys/trusted-keys/trusted_tpm2.c:383:8: error: use of undeclared label 'err'
                   goto err;
                        ^
>> security/keys/trusted-keys/trusted_tpm2.c:428:21: error: expected parameter declarator
           tpm_buf_append_u32(&buf, options->keyhandle);
                              ^
>> 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);
                             ^
>> security/keys/trusted-keys/trusted_tpm2.c:428:2: warning: declaration specifier missing, defaulting to 'int'
           tpm_buf_append_u32(&buf, options->keyhandle);
           ^
           int
>> 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
>> 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,
                               ^
>> 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
>> 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,
                      ^
>> 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
>> security/keys/trusted-keys/trusted_tpm2.c:442:24: error: use of undeclared identifier 'chip'
           rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
                                 ^
>> 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
>> 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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29629 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tpm2_load_command leaks memory
  2021-06-10  9:49 [PATCH] tpm2_load_command leaks memory Dhiraj Shah
  2021-06-16 14:40 ` kernel test robot
@ 2021-06-16 14:49 ` James Bottomley
  1 sibling, 0 replies; 3+ messages in thread
From: James Bottomley @ 2021-06-16 14:49 UTC (permalink / raw)
  To: Dhiraj Shah
  Cc: Jarkko Sakkinen, Mimi Zohar, David Howells, James Morris,
	Serge E. Hallyn, linux-integrity, keyrings,
	linux-security-module, linux-kernel

On Thu, 2021-06-10 at 10:49 +0100, Dhiraj Shah wrote:
> tpm2_key_decode allocates memory which is stored in blob and it's not
> freed.
> 
> Signed-off-by: Dhiraj Shah <find.dhiraj@gmail.com>
> ---
>  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.

>  	/* 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



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-16 14:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10  9:49 [PATCH] tpm2_load_command leaks memory Dhiraj Shah
2021-06-16 14:40 ` kernel test robot
2021-06-16 14:49 ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).