* [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() @ 2016-10-28 2:18 Simon Glass 2016-10-28 18:59 ` Tom Rini 0 siblings, 1 reply; 5+ messages in thread From: Simon Glass @ 2016-10-28 2:18 UTC (permalink / raw) To: u-boot 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; } -- 2.8.0.rc3.226.g39d4020 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() 2016-10-28 2:18 [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() Simon Glass @ 2016-10-28 18:59 ` Tom Rini 2016-10-28 19:41 ` Simon Glass 0 siblings, 1 reply; 5+ messages in thread From: Tom Rini @ 2016-10-28 18:59 UTC (permalink / raw) To: u-boot 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? -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161028/6a2dd807/attachment.sig> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() 2016-10-28 18:59 ` Tom Rini @ 2016-10-28 19:41 ` Simon Glass 2016-10-28 20:04 ` Tom Rini 0 siblings, 1 reply; 5+ messages in thread From: Simon Glass @ 2016-10-28 19:41 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() 2016-10-28 19:41 ` Simon Glass @ 2016-10-28 20:04 ` Tom Rini 2016-10-31 16:13 ` Simon Glass 0 siblings, 1 reply; 5+ messages in thread From: Tom Rini @ 2016-10-28 20:04 UTC (permalink / raw) To: u-boot 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 :) -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161028/0350df60/attachment.sig> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() 2016-10-28 20:04 ` Tom Rini @ 2016-10-31 16:13 ` Simon Glass 0 siblings, 0 replies; 5+ messages in thread From: Simon Glass @ 2016-10-31 16:13 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-31 16:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-28 2:18 [U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg() Simon Glass 2016-10-28 18:59 ` Tom Rini 2016-10-28 19:41 ` Simon Glass 2016-10-28 20:04 ` Tom Rini 2016-10-31 16:13 ` Simon Glass
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox