[PATCH 1/2] image-host: add a check of the return value of snprintf.

[Simon Glass]

Hi Hugo,

On Fri, Dec 29, 2023 at 2:07 PM Hugo Cornelis
<hugo.cornelis at essensium.com> wrote:
>
> This patch allows to generate a sensible error message when generating
> binary images using very long filenames.
>
> This can happen with Buildroot and Yocto builds.
>
> Signed-off-by: Hugo Cornelis <hugo.cornelis at essensium.com>
> ---
>
>  tools/image-host.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>

Good to see this, but I would like a refactoring, please see below.

> diff --git a/tools/image-host.c b/tools/image-host.c
> index ca4950312f..3719f36117 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -378,6 +378,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info,
>         char *algo_name;
>         char filename[128];
>         int ret = -1;
> +       int snprintf_return;

sret ? We try to use 'ret' for return values. But really I would
prefer if you could split this function in two, since it is too long.
Then you probably can use 'ret' in both functions.

>
>         if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) {
>                 fprintf(stderr, "Can't get algo name for cipher in image '%s'\n",
> @@ -414,8 +415,18 @@ static int fit_image_setup_cipher(struct image_cipher_info *info,
>         }
>
>         /* Read the key in the file */
> -       snprintf(filename, sizeof(filename), "%s/%s%s",
> -                info->keydir, info->keyname, ".bin");
> +       snprintf_return = snprintf(filename, sizeof(filename), "%s/%s%s",
> +                                  info->keydir, info->keyname, ".bin");
> +       if (snprintf_return >= sizeof(filename)) {
> +               printf("Can't format the key filename when setting up the cipher: insufficient buffer space\n");
> +               ret = -1;
> +               goto out;
> +       }
> +       if (snprintf_return < 0) {
> +               printf("Can't format the key filename when setting up the cipher: snprintf error\n");
> +               ret = -1;
> +               goto out;
> +       }
>         info->key = malloc(info->cipher->key_len);
>         if (!info->key) {
>                 fprintf(stderr, "Can't allocate memory for key\n");
> @@ -436,8 +447,18 @@ static int fit_image_setup_cipher(struct image_cipher_info *info,
>
>         if (info->ivname) {
>                 /* Read the IV in the file */
> -               snprintf(filename, sizeof(filename), "%s/%s%s",
> -                        info->keydir, info->ivname, ".bin");
> +               snprintf_return = snprintf(filename, sizeof(filename), "%s/%s%s",
> +                                          info->keydir, info->ivname, ".bin");
> +               if (snprintf_return >= sizeof(filename)) {
> +                       printf("Can't format the IV filename when setting up the cipher: insufficient buffer space\n");
> +                       ret = -1;
> +                       goto out;
> +               }
> +               if (snprintf_return < 0) {
> +                       printf("Can't format the IV filename when setting up the cipher: snprintf error\n");
> +                       ret = -1;
> +                       goto out;
> +               }
>                 ret = fit_image_read_data(filename, (unsigned char *)info->iv,
>                                           info->cipher->iv_len);
>         } else {
> --
> 2.34.1
>

Regards,
Simon