Xen devel - Sep 2013 - [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

Coverity complains about the use of strncpy() to completely fill the anchor
strings, resulting in an unterminated string.

Although the strncpy result is correct, the anchor strings are not strings in
the C sense, and use of memcpy is the prevaling style elsewhere in hvmloader
anyway.

While tidying up the style in this function, also remove some trailing
whitespace and gratuitous cast.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 tools/firmware/hvmloader/smbios.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/firmware/hvmloader/smbios.c
b/tools/firmware/hvmloader/smbios.c
index 9f292cc..900f4e7 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -347,18 +347,19 @@ smbios_entry_point_init(void *start,
 {
     uint8_t sum;
     int i;
-    struct smbios_entry_point *ep = (struct smbios_entry_point *)start;
+    struct smbios_entry_point *ep = start;
 
     memset(ep, 0, sizeof(*ep));
 
-    strncpy(ep->anchor_string, "_SM_", 4);
+    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
     ep->length = 0x1f;
     ep->smbios_major_version = 2;
     ep->smbios_minor_version = 4;
     ep->max_structure_size = max_structure_size;
     ep->entry_point_revision = 0;
-    strncpy(ep->intermediate_anchor_string, "_DMI_", 5);
-    
+    memcpy(ep->intermediate_anchor_string, "_DMI_",
+           sizeof(ep->intermediate_anchor_string));
+
     ep->structure_table_length = structure_table_length;
     ep->structure_table_address = structure_table_address;
     ep->number_of_structures = number_of_structures;
-- 
1.7.10.4

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Andrew Cooper
> Sent: 23 September 2013 19:22
> To: Xen-devel
> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
> Subject: [Xen-devel] [PATCH] hvmloader/smbios: Change strncpy to
> memcpy for anchor strings.
> 
> Coverity complains about the use of strncpy() to completely fill the anchor
> strings, resulting in an unterminated string.
> 
> Although the strncpy result is correct, the anchor strings are not strings
in
> the C sense, and use of memcpy is the prevaling style elsewhere in
> hvmloader
> anyway.
> 
> While tidying up the style in this function, also remove some trailing
> whitespace and gratuitous cast.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  tools/firmware/hvmloader/smbios.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/smbios.c
> b/tools/firmware/hvmloader/smbios.c
> index 9f292cc..900f4e7 100644
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -347,18 +347,19 @@ smbios_entry_point_init(void *start,
>  {
>      uint8_t sum;
>      int i;
> -    struct smbios_entry_point *ep = (struct smbios_entry_point *)start;
> +    struct smbios_entry_point *ep = start;
> 
>      memset(ep, 0, sizeof(*ep));
> 
> -    strncpy(ep->anchor_string, "_SM_", 4);
> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
Why the change from 4 to sizeof(ep->anchor_string) here (and similar below)?
Setting the copy length based on the size of the destination rather than the
source seems like the wrong thing to do.

  Paul
>      ep->length = 0x1f;
>      ep->smbios_major_version = 2;
>      ep->smbios_minor_version = 4;
>      ep->max_structure_size = max_structure_size;
>      ep->entry_point_revision = 0;
> -    strncpy(ep->intermediate_anchor_string, "_DMI_", 5);
> -
> +    memcpy(ep->intermediate_anchor_string, "_DMI_",
> +           sizeof(ep->intermediate_anchor_string));
> +
>      ep->structure_table_length = structure_table_length;
>      ep->structure_table_address = structure_table_address;
>      ep->number_of_structures = number_of_structures;
> --
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 24.09.13 at 11:14, Paul Durrant <Paul.Durrant@citrix.com>
wrote:
>>  -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 23 September 2013 19:22
>> To: Xen-devel
>> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
>> Subject: [Xen-devel] [PATCH] hvmloader/smbios: Change strncpy to
>> memcpy for anchor strings.
>> 
>> Coverity complains about the use of strncpy() to completely fill the
anchor
>> strings, resulting in an unterminated string.
>> 
>> Although the strncpy result is correct, the anchor strings are not
strings
> in
>> the C sense, and use of memcpy is the prevaling style elsewhere in
>> hvmloader
>> anyway.
>> 
>> While tidying up the style in this function, also remove some trailing
>> whitespace and gratuitous cast.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  tools/firmware/hvmloader/smbios.c |    9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/firmware/hvmloader/smbios.c
>> b/tools/firmware/hvmloader/smbios.c
>> index 9f292cc..900f4e7 100644
>> --- a/tools/firmware/hvmloader/smbios.c
>> +++ b/tools/firmware/hvmloader/smbios.c
>> @@ -347,18 +347,19 @@ smbios_entry_point_init(void *start,
>>  {
>>      uint8_t sum;
>>      int i;
>> -    struct smbios_entry_point *ep = (struct smbios_entry_point
*)start;
>> +    struct smbios_entry_point *ep = start;
>> 
>>      memset(ep, 0, sizeof(*ep));
>> 
>> -    strncpy(ep->anchor_string, "_SM_", 4);
>> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
> 
> Why the change from 4 to sizeof(ep->anchor_string) here (and similar
below)?
> Setting the copy length based on the size of the destination rather than
the
> source seems like the wrong thing to do.
One can argue either way here:
- passing the destination''s size guarantees no memory corruption
- passing the source''s size guarantees no uninitialized memory

Since the structure fields involved here aren''t going to change,
either way is fine imo.

Jan

>>> On 23.09.13 at 20:22, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Coverity complains about the use of strncpy() to completely fill the anchor
> strings, resulting in an unterminated string.
> 
> Although the strncpy result is correct, the anchor strings are not strings 
> in
> the C sense, and use of memcpy is the prevaling style elsewhere in
hvmloader
> anyway.
> 
> While tidying up the style in this function, also remove some trailing
> whitespace and gratuitous cast.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
Reviewed by: Jan Beulich <jbeulich@suse.com>
> ---
>  tools/firmware/hvmloader/smbios.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/smbios.c 
> b/tools/firmware/hvmloader/smbios.c
> index 9f292cc..900f4e7 100644
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -347,18 +347,19 @@ smbios_entry_point_init(void *start,
>  {
>      uint8_t sum;
>      int i;
> -    struct smbios_entry_point *ep = (struct smbios_entry_point *)start;
> +    struct smbios_entry_point *ep = start;
>  
>      memset(ep, 0, sizeof(*ep));
>  
> -    strncpy(ep->anchor_string, "_SM_", 4);
> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
>      ep->length = 0x1f;
>      ep->smbios_major_version = 2;
>      ep->smbios_minor_version = 4;
>      ep->max_structure_size = max_structure_size;
>      ep->entry_point_revision = 0;
> -    strncpy(ep->intermediate_anchor_string, "_DMI_", 5);
> -    
> +    memcpy(ep->intermediate_anchor_string, "_DMI_",
> +           sizeof(ep->intermediate_anchor_string));
> +
>      ep->structure_table_length = structure_table_length;
>      ep->structure_table_address = structure_table_address;
>      ep->number_of_structures = number_of_structures;
> -- 
> 1.7.10.4

On 24/09/2013 10:39, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> -    strncpy(ep->anchor_string, "_SM_", 4);
>>> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
>> 
>> Why the change from 4 to sizeof(ep->anchor_string) here (and similar
below)?
>> Setting the copy length based on the size of the destination rather
than the
>> source seems like the wrong thing to do.
> 
> One can argue either way here:
> - passing the destination''s size guarantees no memory corruption
> - passing the source''s size guarantees no uninitialized memory
> 
> Since the structure fields involved here aren''t going to change,
> either way is fine imo.
As was the unadorned number 4, imo.

 -- Keir

>>> On 24.09.13 at 12:11, Keir Fraser <keir.xen@gmail.com> wrote:
> On 24/09/2013 10:39, "Jan Beulich" <JBeulich@suse.com>
wrote:
> 
>>>> -    strncpy(ep->anchor_string, "_SM_", 4);
>>>> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
>>> 
>>> Why the change from 4 to sizeof(ep->anchor_string) here (and
similar below)?
>>> Setting the copy length based on the size of the destination rather
than the
>>> source seems like the wrong thing to do.
>> 
>> One can argue either way here:
>> - passing the destination''s size guarantees no memory
corruption
>> - passing the source''s size guarantees no uninitialized memory
>> 
>> Since the structure fields involved here aren''t going to
change,
>> either way is fine imo.
> 
> As was the unadorned number 4, imo.
Yes - I''m fine with both; I don''t even favor one over the
other.

Jan

On 24/09/13 11:11, Keir Fraser wrote:
> On 24/09/2013 10:39, "Jan Beulich" <JBeulich@suse.com>
wrote:
>
>>>> -    strncpy(ep->anchor_string, "_SM_", 4);
>>>> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
>>> Why the change from 4 to sizeof(ep->anchor_string) here (and
similar below)?
>>> Setting the copy length based on the size of the destination rather
than the
>>> source seems like the wrong thing to do.
>> One can argue either way here:
>> - passing the destination''s size guarantees no memory
corruption
>> - passing the source''s size guarantees no uninitialized memory
>>
>> Since the structure fields involved here aren''t going to
change,
>> either way is fine imo.
> As was the unadorned number 4, imo.
>
>  -- Keir
>
>
So what is the verdict here?  I changed 4 to sizeof to match the
prevailing style of other anchor strings in hvmloader.

I can resubmit and change back to hard coded numbers if that would cause
the patch to be accepted.

~Andrew

On 26/09/2013 14:52, "Andrew Cooper" <andrew.cooper3@citrix.com>
wrote:
>>>>> -    strncpy(ep->anchor_string, "_SM_", 4);
>>>>> +    memcpy(ep->anchor_string, "_SM_",
sizeof(ep->anchor_string));
>>>> Why the change from 4 to sizeof(ep->anchor_string) here (and
similar
>>>> below)?
>>>> Setting the copy length based on the size of the destination
rather than
>>>> the
>>>> source seems like the wrong thing to do.
>>> One can argue either way here:
>>> - passing the destination''s size guarantees no memory
corruption
>>> - passing the source''s size guarantees no uninitialized
memory
>>> 
>>> Since the structure fields involved here aren''t going to
change,
>>> either way is fine imo.
>> As was the unadorned number 4, imo.
>> 
>>  -- Keir
>> 
>> 
> 
> So what is the verdict here?  I changed 4 to sizeof to match the
> prevailing style of other anchor strings in hvmloader.
> 
> I can resubmit and change back to hard coded numbers if that would cause
> the patch to be accepted.
Oh I don''t care that much, though I would have left as hard coded
numbers.

Did I give an Ack yet? Here you can have one:
Acked-by: Keir Fraser <keir@xen.org>

 -- Keir