[U-Boot] [PATCH v3 2/2] image: Protect against overflow in unknown_msg()

[Tom Rini <trini@konsulko.com>]

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>