[U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg()
Coverity complains that this can overflow. If we later increase the size of one of the strings in the table, it could happen.
Adjust the code to protect against this.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Coverity (CID: 150964) ---
Changes in v3: - Adjust to deal with what strncpy() actually does (I think)
Changes in v2: - Drop unwanted #include
common/image.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/image.c b/common/image.c index 0e86c13..016f263 100644 --- a/common/image.c +++ b/common/image.c @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const table_entry_t *table, int id) static const char *unknown_msg(enum ih_category category) { static char msg[30]; + static char unknown_str = "Unknown ";
- strcpy(msg, "Unknown "); - strcat(msg, table_info[category].desc); + strcpy(msg, unknown_str); + strncat(msg, table_info[category].desc, + sizeof(msg) - sizeof(unknown_str));
return msg; }
On Thu, Oct 27, 2016 at 08:18:39PM -0600, Simon Glass wrote:
Coverity complains that this can overflow. If we later increase the size of one of the strings in the table, it could happen.
Adjust the code to protect against this.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Coverity (CID: 150964)
Changes in v3:
- Adjust to deal with what strncpy() actually does (I think)
Changes in v2:
- Drop unwanted #include
common/image.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/image.c b/common/image.c index 0e86c13..016f263 100644 --- a/common/image.c +++ b/common/image.c @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const table_entry_t *table, int id) static const char *unknown_msg(enum ih_category category) { static char msg[30];
- static char unknown_str = "Unknown ";
- strcpy(msg, "Unknown ");
- strcat(msg, table_info[category].desc);
- strcpy(msg, unknown_str);
- strncat(msg, table_info[category].desc,
sizeof(msg) - sizeof(unknown_str));
We still need to subtract 1 more here at the end, for the NUL don't we?
Hi Tom,
On 28 October 2016 at 11:59, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 27, 2016 at 08:18:39PM -0600, Simon Glass wrote:
Coverity complains that this can overflow. If we later increase the size of one of the strings in the table, it could happen.
Adjust the code to protect against this.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Coverity (CID: 150964)
Changes in v3:
- Adjust to deal with what strncpy() actually does (I think)
Changes in v2:
- Drop unwanted #include
common/image.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/image.c b/common/image.c index 0e86c13..016f263 100644 --- a/common/image.c +++ b/common/image.c @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const table_entry_t *table, int id) static const char *unknown_msg(enum ih_category category) { static char msg[30];
static char unknown_str = "Unknown ";
strcpy(msg, "Unknown ");strcat(msg, table_info[category].desc);
strcpy(msg, unknown_str);strncat(msg, table_info[category].desc,sizeof(msg) - sizeof(unknown_str));We still need to subtract 1 more here at the end, for the NUL don't we?
I was hoping that the sizeof(msg) would take care of that?
Regards, Simon
On Fri, Oct 28, 2016 at 12:41:05PM -0700, Simon Glass wrote:
Hi Tom,
On 28 October 2016 at 11:59, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 27, 2016 at 08:18:39PM -0600, Simon Glass wrote:
Coverity complains that this can overflow. If we later increase the size of one of the strings in the table, it could happen.
Adjust the code to protect against this.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Coverity (CID: 150964)
Changes in v3:
- Adjust to deal with what strncpy() actually does (I think)
Changes in v2:
- Drop unwanted #include
common/image.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/image.c b/common/image.c index 0e86c13..016f263 100644 --- a/common/image.c +++ b/common/image.c @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const table_entry_t *table, int id) static const char *unknown_msg(enum ih_category category) { static char msg[30];
static char unknown_str = "Unknown ";
strcpy(msg, "Unknown ");strcat(msg, table_info[category].desc);
strcpy(msg, unknown_str);strncat(msg, table_info[category].desc,sizeof(msg) - sizeof(unknown_str));We still need to subtract 1 more here at the end, for the NUL don't we?
I was hoping that the sizeof(msg) would take care of that?
No, and you didn't compile test this did you? ;) I was trying to throw up a stupid test to confirm what all everything would be. test.c:6:28: warning: initialization makes integer from pointer without a cast [-Wint-conversion] static char unknown_str = "Unknown "; ^~~~~~~~~~ test.c:6:28: error: initializer element is not computable at load time
Correcting that to be *unknown_str gives us back the sizeof(char *) not the string in question. So we need to use strlen(unknown_str), and strlen does not include the NUL, so we would need to still add in the - 1 after all of the above.
And I'm being verbose above because string handling can be annoying and I didn't get it 100% right in my head so I figured it's worth showing the work. And since we're going with "Coverity says we've got string problems" we should really correct it, and not just be wrong in a different way :)
Hi Tom,
On 28 October 2016 at 14:04, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 28, 2016 at 12:41:05PM -0700, Simon Glass wrote:
Hi Tom,
On 28 October 2016 at 11:59, Tom Rini trini@konsulko.com wrote:
On Thu, Oct 27, 2016 at 08:18:39PM -0600, Simon Glass wrote:
Coverity complains that this can overflow. If we later increase the size of one of the strings in the table, it could happen.
Adjust the code to protect against this.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Coverity (CID: 150964)
Changes in v3:
- Adjust to deal with what strncpy() actually does (I think)
Changes in v2:
- Drop unwanted #include
common/image.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/common/image.c b/common/image.c index 0e86c13..016f263 100644 --- a/common/image.c +++ b/common/image.c @@ -588,9 +588,11 @@ const table_entry_t *get_table_entry(const table_entry_t *table, int id) static const char *unknown_msg(enum ih_category category) { static char msg[30];
static char unknown_str = "Unknown ";
strcpy(msg, "Unknown ");strcat(msg, table_info[category].desc);
strcpy(msg, unknown_str);strncat(msg, table_info[category].desc,sizeof(msg) - sizeof(unknown_str));We still need to subtract 1 more here at the end, for the NUL don't we?
I was hoping that the sizeof(msg) would take care of that?
No, and you didn't compile test this did you? ;) I was trying to throw up a stupid test to confirm what all everything would be. test.c:6:28: warning: initialization makes integer from pointer without a cast [-Wint-conversion] static char unknown_str = "Unknown "; ^~~~~~~~~~ test.c:6:28: error: initializer element is not computable at load time
Correcting that to be *unknown_str gives us back the sizeof(char *) not the string in question. So we need to use strlen(unknown_str), and strlen does not include the NUL, so we would need to still add in the - 1 after all of the above.
Sorry I was travelling last week and not really focussed on this. I meant to use [].
And I'm being verbose above because string handling can be annoying and I didn't get it 100% right in my head so I figured it's worth showing the work. And since we're going with "Coverity says we've got string problems" we should really correct it, and not just be wrong in a different way :)
I think I can use sizeof() and this time I've tested that it does the right thing, by adding a test function to call it.
Regards, Simon
participants (2)
-
Simon Glass -
Tom Rini