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

[Quentin Schulz]

Hi,

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