[PATCH] scripts: Fix potential null-deref

[PATCH] scripts: Fix potential null-deref

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 scripts/kconfig/menu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 5c5c1374..a0d0d2af 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -812,8 +812,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
 			}
 		}
 	}
-	for_all_prompts(sym, prop)
-		get_prompt_str(r, prop, head);
+	if (sym) {
+		for_all_prompts(sym, prop)
+			get_prompt_str(r, prop, head);
+	}
 
 	prop = get_symbol_prop(sym);
 	if (prop) {
-- 
2.34.1

[PATCH] tools: Fix memory and descriptor leak

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 tools/zynqmpbif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 82ce0ac1..76b7a35f 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -226,8 +226,10 @@ static char *read_full_file(const char *filename, size_t *size)
 	bufp = buf;
 	while (len < sbuf.st_size) {
 		r = read(fd, bufp, sbuf.st_size - len);
-		if (r < 0)
+		if (r < 0) {
+			free(buf);
 			return NULL;
+		}
 		len += r;
 		bufp += r;
 	}
@@ -793,6 +795,8 @@ static const struct bif_file_type *get_file_type(struct bif_entry *entry)
 
 	if (read(fd, &header, sizeof(header)) != sizeof(header)) {
 		printf("Error reading file %s", entry->filename);
+		if (fd)
+			close(fd);
 		return NULL;
 	}
 
-- 
2.34.1

Re: [PATCH] tools: Fix memory and descriptor leak

[Quentin Schulz]

Hi,

On 4/18/25 10:19 AM, ant.v.moryakov@gmail.com wrote:
> From: Maks Mishin <maks.mishinFZ@gmail.com>
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>

Same remark as for the first patch in this series, are you Maks? If no, 
we need your Signed-off-by too.

> ---
>   tools/zynqmpbif.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
> index 82ce0ac1..76b7a35f 100644
> --- a/tools/zynqmpbif.c
> +++ b/tools/zynqmpbif.c
> @@ -226,8 +226,10 @@ static char *read_full_file(const char *filename, size_t *size)
>   	bufp = buf;
>   	while (len < sbuf.st_size) {
>   		r = read(fd, bufp, sbuf.st_size - len);
> -		if (r < 0)
> +		if (r < 0) {
> +			free(buf);
>   			return NULL;

Shouldn't we close the file descriptor too?

Looking at the code a bit more, it seems like we should be closing the 
file descriptor in other error code paths as well.

I would recommend to go for a goto: instead.

e.g.:

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 82ce0ac1a52..0b82f349758 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -226,12 +226,16 @@ static char *read_full_file(const char *filename, 
size_t *size)
  	bufp = buf;
  	while (len < sbuf.st_size) {
  		r = read(fd, bufp, sbuf.st_size - len);
-		if (r < 0)
-			return NULL;
+		if (r < 0) {
+			free(buf);
+			buf = NULL;
+			goto out:
+		}
  		len += r;
  		bufp += r;
  	}

+out:
  	close(fd);

  	return buf;


Then, another patch on top of that, which closes the file descriptor for 
other error code paths as well, e.g.:

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 0b82f349758..81ca9cc7844 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -205,7 +205,7 @@ static const struct bif_flags bif_flags[] = {

  static char *read_full_file(const char *filename, size_t *size)
  {
-	char *buf, *bufp;
+	char *buf = NULL, *bufp;
  	struct stat sbuf;
  	int len = 0, r, fd;

@@ -214,14 +214,14 @@ static char *read_full_file(const char *filename, 
size_t *size)
  		return NULL;

  	if (fstat(fd, &sbuf) < 0)
-		return NULL;
+		goto out;

  	if (size)
  		*size = sbuf.st_size;

  	buf = malloc(sbuf.st_size);
  	if (!buf)
-		return NULL;
+		goto out;

  	bufp = buf;
  	while (len < sbuf.st_size) {


> +		}
>   		len += r;
>   		bufp += r;
>   	}
> @@ -793,6 +795,8 @@ static const struct bif_file_type *get_file_type(struct bif_entry *entry)
>   
>   	if (read(fd, &header, sizeof(header)) != sizeof(header)) {
>   		printf("Error reading file %s", entry->filename);
> +		if (fd)
> +			close(fd);

Fixing a different issue, please in a separate patch.

It seems very wrong to me to close a file descriptor in another function 
that opened it, is this really something we want to be doing? I know we 
already are doing it, (a few lines after that, close(fd) is called), but 
should we "fix" it this way?

Cheers,
Quentin

[PATCH] tools: Fix potential memory leak in aisimage.c

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 tools/aisimage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/aisimage.c b/tools/aisimage.c
index b8b3ee32..6091c8d5 100644
--- a/tools/aisimage.c
+++ b/tools/aisimage.c
@@ -346,6 +346,7 @@ static int aisimage_generate(struct image_tool_params *params,
 		}
 
 	}
+	free(line);
 	fclose(fd);
 
 	aishdr = ais_copy_image(params, aishdr);
-- 
2.34.1

Re: [PATCH] tools: Fix potential memory leak in aisimage.c

[Quentin Schulz]

Hi,

On 4/18/25 10:19 AM, ant.v.moryakov@gmail.com wrote:
> From: Maks Mishin <maks.mishinFZ@gmail.com>
> 

We don't really like empty commit logs, could you please provide some 
information on what this fixes and how?

e.g. here you could say something along the lines of:

line gets realloc'ed in getline() so we need to free it.

> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>

Same remark as other patches in this series, are you Maks? If no, we 
need an additional Signed-off-by, yours.

> ---
>   tools/aisimage.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tools/aisimage.c b/tools/aisimage.c
> index b8b3ee32..6091c8d5 100644
> --- a/tools/aisimage.c
> +++ b/tools/aisimage.c
> @@ -346,6 +346,7 @@ static int aisimage_generate(struct image_tool_params *params,
>   		}
>   
>   	}
> +	free(line);
>   	fclose(fd);
>   
>   	aishdr = ais_copy_image(params, aishdr);

The change seems fine to me though.

Cheers,
Quentin

[PATCH] tools: image-host: Fix potential memory leak

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 tools/image-host.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 4a24dee8..6b17b810 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -1024,10 +1024,13 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
 	int ret;
 
 	node_name = fit_get_name(fit, noffset, NULL);
-	if (fit_config_get_regions(fit, conf_noffset, noffset, &region,
+	ret = fit_config_get_regions(fit, conf_noffset, noffset, &region,
 				   &region_count, &region_prop,
-				   &region_proplen))
+				   &region_proplen);
+	if (ret) {
+		free(region_prop);
 		return -1;
+	}
 
 	if (fit_image_setup_sig(&info, keydir, keyfile, fit, conf_name, noffset,
 				require_keys ? "conf" : NULL, engine_id,
-- 
2.34.1

Re: [PATCH] tools: image-host: Fix potential memory leak

[Quentin Schulz]

Hi,

On 4/18/25 10:19 AM, ant.v.moryakov@gmail.com wrote:
> From: Maks Mishin <maks.mishinFZ@gmail.com>
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>   tools/image-host.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 4a24dee8..6b17b810 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -1024,10 +1024,13 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
>   	int ret;
>   
>   	node_name = fit_get_name(fit, noffset, NULL);
> -	if (fit_config_get_regions(fit, conf_noffset, noffset, &region,
> +	ret = fit_config_get_regions(fit, conf_noffset, noffset, &region,
>   				   &region_count, &region_prop,
> -				   &region_proplen))
> +				   &region_proplen);
> +	if (ret) {
> +		free(region_prop);

We have a handful of other error code paths after that that would need 
to free region_prop as well.

value is likely not freed "enough" as well, please check.

Same for region.

I would suggest possibly something like:

diff --git a/tools/image-host.c b/tools/image-host.c
index a9b86902763..f97af2109ad 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -1076,51 +1076,54 @@ static int fit_config_process_sig(const char 
*keydir, const char *keyfile,
  {
  	struct image_sign_info info;
  	const char *node_name;
-	struct image_region *region;
-	char *region_prop;
+	struct image_region *region = NULL;
+	char *region_prop = NULL;
  	int region_proplen;
  	int region_count;
-	uint8_t *value;
+	uint8_t *value = NULL;
  	uint value_len;
  	int ret;

  	node_name = fit_get_name(fit, noffset, NULL);
  	if (fit_config_get_regions(fit, conf_noffset, noffset, &region,
  				   &region_count, &region_prop,
-				   &region_proplen))
-		return -1;
+				   &region_proplen)) {
+		ret = -1;
+		goto out;
+	}

  	if (fit_image_setup_sig(&info, keydir, keyfile, fit, conf_name, noffset,
  				require_keys ? "conf" : NULL, engine_id,
-				algo_name))
-		return -1;
+				algo_name)) {
+		ret = -1;
+		goto out;
+	}

  	ret = info.crypto->sign(&info, region, region_count, &value,
  				&value_len);
-	free(region);
  	if (ret) {
  		fprintf(stderr, "Failed to sign '%s' signature node in '%s' conf 
node\n",
  			node_name, conf_name);

  		/* We allow keys to be missing */
-		if (ret == -ENOENT)
-			return 0;
-		return -1;
+		ret = (ret == -ENOENT) ? 0 : -1;
+		goto out;
  	}

  	ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
  				  region_prop, region_proplen, cmdname,
  				  algo_name);
  	if (ret) {
-		if (ret == -FDT_ERR_NOSPACE)
-			return -ENOSPC;
+		if (ret == -FDT_ERR_NOSPACE) {
+			ret = -ENOSPC;
+			goto out;
+		}
  		fprintf(stderr,
  			"Can't write signature for '%s' signature node in '%s' conf node: 
%s\n",
  			node_name, conf_name, fdt_strerror(ret));
-		return -1;
+		ret = -1;
+		goto out;
  	}
-	free(value);
-	free(region_prop);

  	/* Get keyname again, as FDT has changed and invalidated our pointer */
  	info.keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL);
@@ -1133,10 +1136,14 @@ static int fit_config_process_sig(const char 
*keydir, const char *keyfile,
  				"Failed to add verification data for '%s' signature node in '%s' 
configuration node\n",
  				node_name, conf_name);
  		}
-		return ret;
  	}

-	return 0;
+out:
+	free(value);
+	free(region_prop);
+	free(region);
+
+	return ret;
  }

  static int fit_config_add_verification_data(const char *keydir,


NOT TESTED!

Cheers,
Quentin

Re: [PATCH] scripts: Fix potential null-deref

[Quentin Schulz]

Hi Maks(?)

On 4/18/25 10:19 AM, ant.v.moryakov@gmail.com wrote:
> From: Maks Mishin <maks.mishinFZ@gmail.com>
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>

This seems to differ from the sender. Are you Maks? If not, you need to 
add your Signed-off-by to fulfill the DCO requirements.

> ---
>   scripts/kconfig/menu.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 5c5c1374..a0d0d2af 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -812,8 +812,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
>   			}
>   		}
>   	}
> -	for_all_prompts(sym, prop)
> -		get_prompt_str(r, prop, head);
> +	if (sym) {
> +		for_all_prompts(sym, prop)
> +			get_prompt_str(r, prop, head);
> +	}
>   
>   	prop = get_symbol_prop(sym);

This one too can do a null-deref.

And same for some other functions after as well.

Wondering if we shouldn't update the macros to not enter the for loops 
if sym is NULL? So we don't have to patch every caller?

Cheers,
Quentin

Re: [PATCH] scripts: Fix potential null-deref

[Tom Rini]

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

On Fri, Apr 18, 2025 at 03:51:42PM +0200, Quentin Schulz wrote:
> Hi Maks(?)
> 
> On 4/18/25 10:19 AM, ant.v.moryakov@gmail.com wrote:
> > From: Maks Mishin <maks.mishinFZ@gmail.com>
> > 
> > Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> 
> This seems to differ from the sender. Are you Maks? If not, you need to add
> your Signed-off-by to fulfill the DCO requirements.
> 
> > ---
> >   scripts/kconfig/menu.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> > index 5c5c1374..a0d0d2af 100644
> > --- a/scripts/kconfig/menu.c
> > +++ b/scripts/kconfig/menu.c
> > @@ -812,8 +812,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
> >   			}
> >   		}
> >   	}
> > -	for_all_prompts(sym, prop)
> > -		get_prompt_str(r, prop, head);
> > +	if (sym) {
> > +		for_all_prompts(sym, prop)
> > +			get_prompt_str(r, prop, head);
> > +	}
> >   	prop = get_symbol_prop(sym);
> 
> This one too can do a null-deref.
> 
> And same for some other functions after as well.
> 
> Wondering if we shouldn't update the macros to not enter the for loops if
> sym is NULL? So we don't have to patch every caller?

The feedback I had with the original patches here was to bring this up
with the linux kernel community first as this comes from them (and
there's a much wider security-focused set or reviewers there). We can
then easily backport fixes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH] tools: Fix memory and descriptor leak

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
---
 tools/zynqmpbif.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 82ce0ac1a52..33c68eac9f3 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -205,7 +205,7 @@ static const struct bif_flags bif_flags[] = {
 
 static char *read_full_file(const char *filename, size_t *size)
 {
-	char *buf, *bufp;
+	char *buf = NULL, *bufp;
 	struct stat sbuf;
 	int len = 0, r, fd;
 
@@ -214,24 +214,28 @@ static char *read_full_file(const char *filename, size_t *size)
 		return NULL;
 
 	if (fstat(fd, &sbuf) < 0)
-		return NULL;
+		goto out;
 
 	if (size)
 		*size = sbuf.st_size;
 
 	buf = malloc(sbuf.st_size);
 	if (!buf)
-		return NULL;
+		goto out;
 
 	bufp = buf;
 	while (len < sbuf.st_size) {
 		r = read(fd, bufp, sbuf.st_size - len);
-		if (r < 0)
-			return NULL;
+		if (r < 0) {
+			free(buf);
+			buf = NULL;
+			goto out;
+		}
 		len += r;
 		bufp += r;
 	}
 
+out:
 	close(fd);
 
 	return buf;
-- 
2.30.2