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

[Simon Glass]

Hi Tom,

On 28 October 2016 at 14:04, Tom Rini <trini at 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 at 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 at 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