[PATCH] image-host: Fix error value paths and emit error messages to stderr.
A recent refactoring in image-host.c messed up the return values of
the function that reads the encryptiong keys. This patch fixes this
and also makes sure that error output goes to stderr instead of to
stdout.
Signed-off-by: Hugo Cornelis
---
tools/image-host.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/image-host.c b/tools/image-host.c
index b2a0f2e6d1..7bfc0cb6b1 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -346,17 +346,17 @@ static int fit_image_read_key_iv_data(const char *keydir,
const char *key_iv_nam
unsigned char *key_iv_data, int
expected_size)
{
char filename[PATH_MAX];
- int ret = -1;
+ int ret;
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;
+ fprintf(stderr, "Can't format the key or IV filename when
setting up the cipher: insufficient buffer space\n");
+ return -1;
}
if (ret < 0) {
- printf("Can't format the key or IV filename when setting up the
cipher: snprintf error\n");
- ret = -1;
+ fprintf(stderr, "Can't format the key or IV filename when
setting up the cipher: snprintf error\n");
+ return -1;
}
ret = fit_image_read_data(filename, key_iv_data, expected_size);
--
2.34.1
[PATCH] image-host: Fix error value paths and emit error messages to stderr.
Signed-off-by: Hugo Cornelis
---
tools/image-host.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/image-host.c b/tools/image-host.c
index b2a0f2e6d1..06b05dcacb 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -351,12 +351,12 @@ static int fit_image_read_key_iv_data(const char *keydir,
const char *key_iv_nam
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;
+ fprintf(stderr, "Can't format the key or IV filename when
setting up the cipher: insufficient buffer space\n");
+ return -1;
}
if (ret < 0) {
- printf("Can't format the key or IV filename when setting up the
cipher: snprintf error\n");
- ret = -1;
+ fprintf(stderr, "Can't format the key or IV filename when
setting up the cipher: snprintf error\n");
+ return -1;
}
ret = fit_image_read_data(filename, key_iv_data, expected_size);
--
2.34.1
Re: Fwd: New Defects reported by Coverity Scan for Das U-Boot
Hi Tom, sorry about that. Please find attached a patch. Can you please review? Thanks, Hugo
[PATCH] image-host: refactor and protect for very long filenames
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
---
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);
--
2.34.1
Re: [PATCH 1/2] image-host: add a check of the return value of snprintf.
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
[PATCH 2/2] image-host: increase path length when setting up the cipher.
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
---
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;
--
2.34.1
[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
---
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 {
--
2.34.1
[PATCH 2/2] image-host: increase path length when setting up the cipher.
---
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 0c92a2ddeb..9afcc02192 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -361,7 +361,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
[PATCH 1/2] image-host: add a check of the return value of snprintf.
---
tools/image-host.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/tools/image-host.c b/tools/image-host.c
index a6b0a94420..0c92a2ddeb 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -363,6 +363,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)) {
printf("Can't get algo name for cipher in image '%s'\n",
@@ -399,8 +400,20 @@ static int fit_image_setup_cipher(struct image_cipher_info
*info,
}
/* Read the key in the file */
- snprintf(filename, sizeof(filename), "%s/%s%s",
+ 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) {
printf("Can't allocate memory for key\n");
@@ -423,6 +436,18 @@ static int fit_image_setup_cipher(struct image_cipher_info
*info,
/* Read the IV in the file */
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
image-host: small improvements and fixes.
Yocto build can involve very long filenames. These two patches protect the Yocto build from failing without a sensible error message and increase the path length for the cipher key for the kernel from 128 to 256 characters.
