* 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