[PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Andrew Cooper]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Paul Durrant]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Jan Beulich]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Keir Fraser]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Jan Beulich]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Andrew Cooper]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Keir Fraser]

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

Re: [PATCH] hvmloader/smbios: Change strncpy to memcpy for anchor strings.

[Jan Beulich]

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