[PATCH 1/2] image-host: add a check of the return value of snprintf.
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@essensium.com ---
tools/image-host.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
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;
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 {
This patch increases the maximum path length of the filename containing the cipher key for the kernel from 128 to 256 characters.
Signed-off-by: Hugo Cornelis hugo.cornelis@essensium.com ---
tools/image-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/image-host.c b/tools/image-host.c index 3719f36117..b0012a714d 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -376,7 +376,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, int noffset) { char *algo_name; - char filename[128]; + char filename[256]; int ret = -1; int snprintf_return;
On Fri, Dec 29, 2023 at 2:07 PM Hugo Cornelis hugo.cornelis@essensium.com wrote:
This patch increases the maximum path length of the filename containing the cipher key for the kernel from 128 to 256 characters.
Signed-off-by: Hugo Cornelis hugo.cornelis@essensium.com
tools/image-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
This is fine, but do you not want to use PATH_MAX ? We should not need to worry about stack space on the host.
diff --git a/tools/image-host.c b/tools/image-host.c index 3719f36117..b0012a714d 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -376,7 +376,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, int noffset) { char *algo_name;
char filename[128];
char filename[256]; int ret = -1; int snprintf_return;-- 2.34.1
Regards, Simon
Hi Hugo,
On Fri, Dec 29, 2023 at 2:07 PM Hugo Cornelis hugo.cornelis@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@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
Hi Simon, thanks for the feedback.
Yes, you were right. I have split the function into two functions. The patched is attached below (inline).
Some of the calls can be further 'logically' reordered, I believe, but I don't know the program flow well enough to be sure.
One other 'inconsistency' is the occurence of a lookup based on the string literal "iv-name-hint" while a few lines further in the source code a lookup is performed based on a pre-processor define rather than a string literal. I have no clue about the reason of this difference.
Further comments are welcome.
Thanks, Hugo
This patch adds a function fit_image_read_key_iv_data that checks the return value of snprintf and allows to generate a sensible error message when generating binary images using filenames that are too long for the OS to handle.
This is especially relevant for automated builds such as Buildroot and Yocto builds.
Signed-off-by: Hugo Cornelis hugo.cornelis@essensium.com --- tools/image-host.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/tools/image-host.c b/tools/image-host.c index ca4950312f..0092fa830f 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -340,6 +340,28 @@ err: return ret; }
+static int fit_image_read_key_iv_data(const char *keydir, const char *key_iv_name, + unsigned char *key_iv_data, int expected_size) +{ + char filename[PATH_MAX]; + int ret = -1; + + ret = snprintf(filename, sizeof(filename), "%s/%s%s", + keydir, key_iv_name, ".bin"); + if (ret >= sizeof(filename)) { + printf("Can't format the key or IV filename when setting up the cipher: insufficient buffer space\n"); + ret = -1; + } + if (ret < 0) { + printf("Can't format the key or IV filename when setting up the cipher: snprintf error\n"); + ret = -1; + } + + ret = fit_image_read_data(filename, key_iv_data, expected_size); + + return ret; +} + static int get_random_data(void *data, int size) { unsigned char *tmp = data; @@ -376,7 +398,6 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, int noffset) { char *algo_name; - char filename[128]; int ret = -1;
if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) { @@ -413,17 +434,17 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, goto out; }
- /* Read the key in the file */ - snprintf(filename, sizeof(filename), "%s/%s%s", - info->keydir, info->keyname, ".bin"); info->key = malloc(info->cipher->key_len); if (!info->key) { fprintf(stderr, "Can't allocate memory for key\n"); ret = -1; goto out; } - ret = fit_image_read_data(filename, (unsigned char *)info->key, - info->cipher->key_len); + + /* Read the key in the file */ + ret = fit_image_read_key_iv_data(info->keydir, info->keyname, + (unsigned char *)info->key, + info->cipher->key_len); if (ret < 0) goto out;
@@ -436,10 +457,11 @@ 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"); - ret = fit_image_read_data(filename, (unsigned char *)info->iv, - info->cipher->iv_len); + ret = fit_image_read_key_iv_data(info->keydir, info->ivname, + (unsigned char *)info->iv, + info->cipher->iv_len); + if (ret < 0) + goto out; } else { /* Generate an ramdom IV */ ret = get_random_data((void *)info->iv, info->cipher->iv_len);
On Mon, Jan 08, 2024 at 03:24:30PM +0100, Hugo Cornelis wrote:
This patch adds a function fit_image_read_key_iv_data that checks the return value of snprintf and allows to generate a sensible error message when generating binary images using filenames that are too long for the OS to handle.
This is especially relevant for automated builds such as Buildroot and Yocto builds.
Signed-off-by: Hugo Cornelis hugo.cornelis@essensium.com
Applied to u-boot/master, thanks!
participants (3)
-
Hugo Cornelis -
Simon Glass -
Tom Rini