[PATCH] KEYS: fix memory leak when reading certificate fails

[PATCH] KEYS: fix memory leak when reading certificate fails

[Denis Glazkov]

In the `read_file` function of `insert-sys-cert.c` script, if
the data is read incorrectly, the memory allocated for the `buf`
array is not freed.

Fixes: c4c361059585 ("KEYS: Reserve an extra certificate symbol for inserting without recompiling")
Signed-off-by: Denis Glazkov <d.glazkov@omp.ru>
---
 scripts/insert-sys-cert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/insert-sys-cert.c b/scripts/insert-sys-cert.c
index 8902836c2342..b98a0b12f16f 100644
--- a/scripts/insert-sys-cert.c
+++ b/scripts/insert-sys-cert.c
@@ -251,6 +251,7 @@ static char *read_file(char *file_name, int *size)
 	if (read(fd, buf, *size) != *size) {
 		perror("File read failed");
 		close(fd);
+		free(buf);
 		return NULL;
 	}
 	close(fd);
-- 
2.25.1

Re: [PATCH] KEYS: fix memory leak when reading certificate fails

[Dongliang Mu]

On Thu, Mar 3, 2022 at 7:49 PM Denis Glazkov <d.glazkov@omp.ru> wrote:
>
> In the `read_file` function of `insert-sys-cert.c` script, if
> the data is read incorrectly, the memory allocated for the `buf`
> array is not freed.
>
> Fixes: c4c361059585 ("KEYS: Reserve an extra certificate symbol for inserting without recompiling")
> Signed-off-by: Denis Glazkov <d.glazkov@omp.ru>
> ---
>  scripts/insert-sys-cert.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/insert-sys-cert.c b/scripts/insert-sys-cert.c
> index 8902836c2342..b98a0b12f16f 100644
> --- a/scripts/insert-sys-cert.c
> +++ b/scripts/insert-sys-cert.c
> @@ -251,6 +251,7 @@ static char *read_file(char *file_name, int *size)
>         if (read(fd, buf, *size) != *size) {
>                 perror("File read failed");
>                 close(fd);
> +               free(buf);
>                 return NULL;
>         }
>         close(fd);

Hi Denis,

There is another issue related to variable buf. On the success path,
buf will be assigned to variable cert in the main function. And cert
is not free when the main function exits.

> --
> 2.25.1

[PATCH v2] KEYS: fix memory leaks when reading certificate

[Denis Glazkov]

The `exit()` function usage produce possible memory leaks. This
patch removes the use of the `exit()` function and adds memory
free in case of a negative scenarios.

Fixes: c4c361059585 ("KEYS: Reserve an extra certificate symbol for inserting without recompiling")
Signed-off-by: Denis Glazkov <d.glazkov@omp.ru>
---
 scripts/insert-sys-cert.c | 51 +++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/scripts/insert-sys-cert.c b/scripts/insert-sys-cert.c
index 8902836c2342..8046682188f3 100644
--- a/scripts/insert-sys-cert.c
+++ b/scripts/insert-sys-cert.c
@@ -101,7 +101,7 @@ static void get_symbol_from_map(Elf_Ehdr *hdr, FILE *f, char *name,
 	s->offset = 0;
 	if (fseek(f, 0, SEEK_SET) != 0) {
 		perror("File seek failed");
-		exit(EXIT_FAILURE);
+		return;
 	}
 	while (fgets(l, LINE_SIZE, f)) {
 		p = strchr(l, '\n');
@@ -251,6 +251,7 @@ static char *read_file(char *file_name, int *size)
 	if (read(fd, buf, *size) != *size) {
 		perror("File read failed");
 		close(fd);
+		free(buf);
 		return NULL;
 	}
 	close(fd);
@@ -277,14 +278,15 @@ int main(int argc, char **argv)
 	char *cert_file = NULL;
 	int vmlinux_size;
 	int cert_size;
-	Elf_Ehdr *hdr;
-	char *cert;
-	FILE *system_map;
+	Elf_Ehdr *hdr = NULL;
+	char *cert = NULL;
+	FILE *system_map = NULL;
 	unsigned long *lsize;
 	int *used;
 	int opt;
 	Elf_Shdr *symtab = NULL;
 	struct sym cert_sym, lsize_sym, used_sym;
+	int ret = EXIT_FAILURE;
 
 	while ((opt = getopt(argc, argv, "b:c:s:")) != -1) {
 		switch (opt) {
@@ -304,20 +306,20 @@ int main(int argc, char **argv)
 
 	if (!vmlinux_file || !cert_file) {
 		print_usage(argv[0]);
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	cert = read_file(cert_file, &cert_size);
 	if (!cert)
-		exit(EXIT_FAILURE);
+		goto finish;
 
 	hdr = map_file(vmlinux_file, &vmlinux_size);
 	if (!hdr)
-		exit(EXIT_FAILURE);
+		goto finish;
 
 	if (vmlinux_size < sizeof(*hdr)) {
 		err("Invalid ELF file.\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	if ((hdr->e_ident[EI_MAG0] != ELFMAG0) ||
@@ -325,22 +327,22 @@ int main(int argc, char **argv)
 	    (hdr->e_ident[EI_MAG2] != ELFMAG2) ||
 	    (hdr->e_ident[EI_MAG3] != ELFMAG3)) {
 		err("Invalid ELF magic.\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	if (hdr->e_ident[EI_CLASS] != CURRENT_ELFCLASS) {
 		err("ELF class mismatch.\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	if (hdr->e_ident[EI_DATA] != endianness()) {
 		err("ELF endian mismatch.\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	if (hdr->e_shoff > vmlinux_size) {
 		err("Could not find section header.\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
 	symtab = get_symbol_table(hdr);
@@ -349,13 +351,13 @@ int main(int argc, char **argv)
 		if (!system_map_file) {
 			err("Please provide a System.map file.\n");
 			print_usage(argv[0]);
-			exit(EXIT_FAILURE);
+			goto finish;
 		}
 
 		system_map = fopen(system_map_file, "r");
 		if (!system_map) {
 			perror(system_map_file);
-			exit(EXIT_FAILURE);
+			goto finish;
 		}
 		get_symbol_from_map(hdr, system_map, CERT_SYM, &cert_sym);
 		get_symbol_from_map(hdr, system_map, USED_SYM, &used_sym);
@@ -371,7 +373,7 @@ int main(int argc, char **argv)
 	}
 
 	if (!cert_sym.offset || !lsize_sym.offset || !used_sym.offset)
-		exit(EXIT_FAILURE);
+		goto finish;
 
 	print_sym(hdr, &cert_sym);
 	print_sym(hdr, &used_sym);
@@ -382,14 +384,16 @@ int main(int argc, char **argv)
 
 	if (cert_sym.size < cert_size) {
 		err("Certificate is larger than the reserved area!\n");
-		exit(EXIT_FAILURE);
+		goto finish;
 	}
 
+	ret = EXIT_SUCCESS;
+
 	/* If the existing cert is the same, don't overwrite */
 	if (cert_size == *used &&
 	    strncmp(cert_sym.content, cert, cert_size) == 0) {
 		warn("Certificate was already inserted.\n");
-		exit(EXIT_SUCCESS);
+		goto finish;
 	}
 
 	if (*used > 0)
@@ -406,5 +410,16 @@ int main(int argc, char **argv)
 						cert_sym.address);
 	info("Used %d bytes out of %d bytes reserved.\n", *used,
 						 cert_sym.size);
-	exit(EXIT_SUCCESS);
+
+finish:
+	if (cert != NULL)
+		free(cert);
+
+	if (hdr != NULL)
+		munmap(hdr, vmlinux_size);
+
+	if (system_map != NULL)
+		fclose(system_map);
+
+	return ret;
 }
-- 
2.25.1

Re: [PATCH v2] KEYS: fix memory leaks when reading certificate

[David Howells]

Denis Glazkov <d.glazkov@omp.ru> wrote:

> The `exit()` function usage produce possible memory leaks. This
> patch removes the use of the `exit()` function and adds memory
> free in case of a negative scenarios.

?

Barring a kernel bug, there should be no memory leaks from exit().  _exit() is
the ultimate process cleanup tool.  Calling free() won't necessarily return
the memory allocated by malloc() to the kernel.

Unless you have a good reason to actually tear down everything, just print a
message and call exit on error in little helpers like this.

David

[PATCH] KEYS: fix memory leak when reading certificate fails

[Denis Glazkov]

In the `read_file` function of `insert-sys-cert.c` script, if
the data is read incorrectly, the memory allocated for the `buf`
array is not freed.

Fixes: c4c361059585 ("KEYS: Reserve an extra certificate symbol for inserting without recompiling")
Signed-off-by: Denis Glazkov <d.glazkov@omp.ru>
---
 scripts/insert-sys-cert.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/insert-sys-cert.c b/scripts/insert-sys-cert.c
index 8902836c2342..b98a0b12f16f 100644
--- a/scripts/insert-sys-cert.c
+++ b/scripts/insert-sys-cert.c
@@ -251,6 +251,7 @@ static char *read_file(char *file_name, int *size)
 	if (read(fd, buf, *size) != *size) {
 		perror("File read failed");
 		close(fd);
+		free(buf);
 		return NULL;
 	}
 	close(fd);
-- 
2.25.1