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

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

[Simon Glass]

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

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

[Tom Rini]

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>

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

[Simon Glass]

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

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

[Tom Rini]

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>

[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@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