[PATCH ima-evm-utils v1 0/4] Fix some issues in evmctl

[PATCH ima-evm-utils v1 0/4] Fix some issues in evmctl

[Stefan Berger]

This series of patches fixes memory leaks related to tpm_bank_info
allocations and entry.template as well as gcc compiler warnings
when building with -fanalyzer (gcc 12.2.1).

This series is intended to be applied on top of Mimi's current series
at https://github.com/mimizohar/ima-evm-utils/tree/next-testing .

   Stefan

Stefan Berger (4):
  Fix memory leaks of tpm_bank_info allocations
  Fix memory leak related to entry.template
  Add assert to ensure that algo_name in bank is set
  Change condition to free(pub)

 src/evmctl.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)


base-commit: f6abaed5d0d0a4478cf25a8096a4ff44be4a234e
-- 
2.38.1

[PATCH ima-evm-utils v1 1/4] Fix memory leaks of tpm_bank_info allocations

[Stefan Berger]

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/evmctl.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 349215e..c2fe152 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1711,8 +1711,10 @@ static struct tpm_bank_info *init_tpm_banks(int *num_banks)
 	int i, j;
 
 	banks = calloc(num_algos, sizeof(struct tpm_bank_info));
-	if (!banks)
-		return banks;
+	if (!banks) {
+		log_err("Out of memory\n");
+		return NULL;
+	}
 
 	/* re-calculate the PCRs digests for only known algorithms */
 	*num_banks = num_algos;
@@ -2083,8 +2085,8 @@ static int read_tpm_banks(int num_banks, struct tpm_bank_info *bank)
 static int ima_measurement(const char *file)
 {
 	struct tpm_bank_info *pseudo_padded_banks;
-	struct tpm_bank_info *pseudo_banks;
-	struct tpm_bank_info *tpm_banks;
+	struct tpm_bank_info *pseudo_banks = NULL;
+	struct tpm_bank_info *tpm_banks = NULL;
 	int is_ima_template, cur_template_fmt;
 	int num_banks = 0;
 	int tpmbanks = 1;
@@ -2102,13 +2104,21 @@ static int ima_measurement(const char *file)
 	memset(zero, 0, MAX_DIGEST_SIZE);
 
 	pseudo_padded_banks = init_tpm_banks(&num_banks);
+	if (!pseudo_padded_banks)
+		return -1;
+
 	pseudo_banks = init_tpm_banks(&num_banks);
+	if (!pseudo_banks)
+		goto out_free;
+
 	tpm_banks = init_tpm_banks(&num_banks);
+	if (!tpm_banks)
+		goto out_free;
 
 	fp = fopen(file, "rb");
 	if (!fp) {
 		log_err("Failed to open measurement file: %s\n", file);
-		return -1;
+		goto out;
 	}
 
 	if (imaevm_params.keyfile)	/* Support multiple public keys */
@@ -2311,6 +2321,11 @@ static int ima_measurement(const char *file)
 
 out:
 	fclose(fp);
+out_free:
+	free(tpm_banks);
+	free(pseudo_banks);
+	free(pseudo_padded_banks);
+
 	return err;
 }
 
@@ -2556,6 +2571,8 @@ static int cmd_ima_bootaggr(struct command *cmd)
 	 */
 	if (file) {
 		tpm_banks = init_tpm_banks(&num_banks);
+		if (!tpm_banks)
+			return -1;
 
 		/* TPM 1.2 only supports SHA1.*/
 		for (i = 1; i < num_banks; i++)
@@ -2565,12 +2582,19 @@ static int cmd_ima_bootaggr(struct command *cmd)
 		if (err) {
 			log_err("Failed reading the TPM 1.2 event log (%s)\n",
 				file);
+			free(tpm_banks);
+
 			return -1;
 		}
 	} else {
 		tpm_banks = init_tpm_banks(&num_banks);
+		if (!tpm_banks)
+			return -1;
+
 		if (read_tpm_banks(num_banks, tpm_banks) != 0) {
 			log_info("Failed to read any TPM PCRs\n");
+			free(tpm_banks);
+
 			return -1;
 		}
 	}
@@ -2604,7 +2628,10 @@ static int cmd_ima_bootaggr(struct command *cmd)
 	}
 	bootaggr[bootaggr_len] = '\0';
 	printf("%s", bootaggr);
+
 	free(bootaggr);
+	free(tpm_banks);
+
 	return 0;
 }
 
-- 
2.38.1

[PATCH ima-evm-utils v1 2/4] Fix memory leak related to entry.template

[Stefan Berger]

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/evmctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index c2fe152..4afc265 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -2095,7 +2095,7 @@ static int ima_measurement(const char *file)
 	unsigned long entry_num = 0;
 	int c;
 
-	struct template_entry entry = { .template = 0 };
+	struct template_entry entry = { .template = NULL };
 	FILE *fp;
 	int invalid_template_digest = 0;
 	int err_padded = -1;
@@ -2206,6 +2206,10 @@ static int ima_measurement(const char *file)
 			free(entry.template);
 			entry.template_buf_len = entry.template_len;
 			entry.template = malloc(entry.template_len);
+			if (!entry.template) {
+				log_err("Out of memory\n");
+				goto out;
+			}
 		}
 
 		if (!is_ima_template) {
@@ -2325,6 +2329,7 @@ out_free:
 	free(tpm_banks);
 	free(pseudo_banks);
 	free(pseudo_padded_banks);
+	free(entry.template);
 
 	return err;
 }
-- 
2.38.1

[PATCH ima-evm-utils v1 3/4] Add assert to ensure that algo_name in bank is set

[Stefan Berger]

To avoid numerous warning messages from gcc 12.2.1 when compiling with
-fanalyzer, insert an assert to ensure that algo_name in each bank
is set. The assert resolves the following warnings:

evmctl.c:1998:30: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]
 1998 |                         if (!strcmp(tpm_banks[j].algo_name, alg)) {
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

evmctl.c: In function ‘ima_measurement’:
evmctl.c:2146:24: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]
 2146 |                     && strcmp(pseudo_padded_banks[c].algo_name, verify_bank)) {
      |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘ima_measurement’: events 1-2

evmctl.c: In function ‘cmd_ima_bootaggr’:
evmctl.c:2611:33: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]
 2611 |                 bootaggr_len += strlen(tpm_banks[i].algo_name) + 1;
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/evmctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/evmctl.c b/src/evmctl.c
index 4afc265..4f55fb6 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1723,6 +1723,7 @@ static struct tpm_bank_info *init_tpm_banks(int *num_banks)
 			if (!strcmp(default_algos[i], hash_algo_name[j]))
 				set_bank_info(&banks[i], hash_algo_name[j]);
 		}
+		assert(banks[i].algo_name);
 	}
 	return banks;
 }
-- 
2.38.1

[PATCH ima-evm-utils v1 4/4] Change condition to free(pub)

[Stefan Berger]

Change the condition under which pub is freed to make it clearer for the
reader and analyzer.

This change gets rid of the following gcc -fanalyzer warning:

evmctl.c:1140:12: warning: leak of ‘pub’ [CWE-401] [-Wanalyzer-malloc-leak]
 1140 |         if (imaevm_params.x509)
      |            ^

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 src/evmctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/evmctl.c b/src/evmctl.c
index 4f55fb6..59a56c8 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1137,7 +1137,7 @@ static int cmd_import(struct command *cmd)
 		log_info("keyid: %d\n", id);
 		printf("%d\n", id);
 	}
-	if (imaevm_params.x509)
+	if (pub != _pub)
 		free(pub);
 	return err;
 }
-- 
2.38.1

Re: [PATCH ima-evm-utils v1 0/4] Fix some issues in evmctl

[Mimi Zohar]

Hi Stefan,

On Wed, 2022-11-02 at 14:45 -0400, Stefan Berger wrote:
> This series of patches fixes memory leaks related to tpm_bank_info
> allocations and entry.template as well as gcc compiler warnings
> when building with -fanalyzer (gcc 12.2.1).
> 
> This series is intended to be applied on top of Mimi's current series
> at https://github.com/mimizohar/ima-evm-utils/tree/next-testing .

Thank you for the cleanup.  Aside from the missing patch descriptions
(1/4, 2/4), the patch set looks good.  It applied cleanly on top of the
"misc bug and other fixes" patch set and Tergel's "support for reading
per bank TPM 2.0 PCRs via sysfs" patch, which are now in next-testing.

-- 
thanks,

Mimi

Re: [PATCH ima-evm-utils v1 0/4] Fix some issues in evmctl

[Mimi Zohar]

Hi Stefan,

On Wed, 2022-11-02 at 14:45 -0400, Stefan Berger wrote:
> This series of patches fixes memory leaks related to tpm_bank_info
> allocations and entry.template as well as gcc compiler warnings
> when building with -fanalyzer (gcc 12.2.1).
> 
> This series is intended to be applied on top of Mimi's current series
> at https://github.com/mimizohar/ima-evm-utils/tree/next-testing .
> 
>    Stefan
> 
> Stefan Berger (4):
>   Fix memory leaks of tpm_bank_info allocations
>   Fix memory leak related to entry.template
>   Add assert to ensure that algo_name in bank is set
>   Change condition to free(pub)

Thanks, Stefan.  The patches applied to the next/next-testing branches.
In the future, no matter how trivial the patch is, please include patch
descriptions.

thanks,

Mimi