[PATCH 0/2] hvmloader: fix two issues spotted by Coverity

[PATCH 0/2] hvmloader: fix two issues spotted by Coverity

[Wei Liu]

Wei Liu (2):
  hvmloader: correctly copy signature to info structures
  hvmloader: cast to 64bit before multiplication in get_module_entry

 tools/firmware/hvmloader/hvmloader.c | 4 ++--
 tools/firmware/hvmloader/ovmf.c      | 3 ++-
 tools/firmware/hvmloader/seabios.c   | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/2] hvmloader: correctly copy signature to info structures

[Wei Liu]

The original code used sizeof(info->signature) as the size parameter for
memcpy, which was wrong.

Fix that by calculating the correct size.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/ovmf.c    | 3 ++-
 tools/firmware/hvmloader/seabios.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c
index b4bcc93..a4ed661 100644
--- a/tools/firmware/hvmloader/ovmf.c
+++ b/tools/firmware/hvmloader/ovmf.c
@@ -67,10 +67,11 @@ struct ovmf_info {
 static void ovmf_setup_bios_info(void)
 {
     struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
+    const char sig[] = "XenHVMOVMF";
 
     memset(info, 0, sizeof(*info));
 
-    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
+    memcpy(info->signature, sig, sizeof(sig));
     info->length = sizeof(*info);
 }
 
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index 5c9a351..ca092cc 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -55,10 +55,11 @@ struct seabios_info {
 static void seabios_setup_bios_info(void)
 {
     struct seabios_info *info = (void *)BIOS_INFO_PHYSICAL_ADDRESS;
+    const char sig[] = "XenHVMSeaBIOS";
 
     memset(info, 0, sizeof(*info));
 
-    memcpy(info->signature, "XenHVMSeaBIOS", sizeof(info->signature));
+    memcpy(info->signature, sig, sizeof(sig));
     info->length = sizeof(*info);
 
     info->tables = (uint32_t)scratch_alloc(MAX_TABLES*sizeof(uint32_t), 0);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Wei Liu]

Coverity complains:

overflow_before_widen: Potentially overflowing expression
info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is
evaluated using 32-bit arithmetic, and then used in a context that
expects an expression of type uint64_t (64 bits, unsigned).

The overflow is unlikely to happen in reality because we only expect a
few modules.

Fix that by casting the variable to 64bit before multiplication to
placate Coverity.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/hvmloader/hvmloader.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c
index 7b32d86..970222c 100644
--- a/tools/firmware/hvmloader/hvmloader.c
+++ b/tools/firmware/hvmloader/hvmloader.c
@@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
 
     if ( !modlist ||
          info->modlist_paddr > UINTPTR_MAX ||
-         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
-            > UINTPTR_MAX )
+         (info->modlist_paddr +
+          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
         return NULL;
 
     for ( i = 0; i < info->nr_modules; i++ )
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures

[Jan Beulich]

>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
> --- a/tools/firmware/hvmloader/ovmf.c
> +++ b/tools/firmware/hvmloader/ovmf.c
> @@ -67,10 +67,11 @@ struct ovmf_info {
>  static void ovmf_setup_bios_info(void)
>  {
>      struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
> +    const char sig[] = "XenHVMOVMF";
>  
>      memset(info, 0, sizeof(*info));
>  
> -    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
> +    memcpy(info->signature, sig, sizeof(sig));
>      info->length = sizeof(*info);
>  }

I think using strncpy() would be more natural in cases like this,
as it would at once make clear that the destination can't be
overrun no matter how large the string literal.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Jan Beulich]

>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
> Coverity complains:
> 
> overflow_before_widen: Potentially overflowing expression
> info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is
> evaluated using 32-bit arithmetic, and then used in a context that
> expects an expression of type uint64_t (64 bits, unsigned).

To me this is Coverity splitting hair, to be honest.

> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
>  
>      if ( !modlist ||
>           info->modlist_paddr > UINTPTR_MAX ||
> -         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
> -            > UINTPTR_MAX )
> +         (info->modlist_paddr +
> +          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
>          return NULL;

This can be had without resorting to 64-bit multiplication, by bounds
checking

         (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist)

instead. While we would certainly hope that compilers don't resort
to a libgcc helper for 64-bit multiplication, I think we'd better avoid
that risk altogether.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures

[Andrew Cooper]

On 19/08/16 09:25, Jan Beulich wrote:
>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
>> --- a/tools/firmware/hvmloader/ovmf.c
>> +++ b/tools/firmware/hvmloader/ovmf.c
>> @@ -67,10 +67,11 @@ struct ovmf_info {
>>  static void ovmf_setup_bios_info(void)
>>  {
>>      struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
>> +    const char sig[] = "XenHVMOVMF";
>>  
>>      memset(info, 0, sizeof(*info));
>>  
>> -    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
>> +    memcpy(info->signature, sig, sizeof(sig));
>>      info->length = sizeof(*info);
>>  }
> I think using strncpy() would be more natural in cases like this,
> as it would at once make clear that the destination can't be
> overrun no matter how large the string literal.

How about structure assignment?

*info = (struct ovmf_info) { .signature = "XenHVMOVMF", .length =
sizeof(*info) }

which also subsumed the memset()?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Andrew Cooper]

On 19/08/16 09:31, Jan Beulich wrote:
>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
>> Coverity complains:
>>
>> overflow_before_widen: Potentially overflowing expression
>> info->nr_modules * 32U with type unsigned int (32 bits, unsigned) is
>> evaluated using 32-bit arithmetic, and then used in a context that
>> expects an expression of type uint64_t (64 bits, unsigned).
> To me this is Coverity splitting hair, to be honest.

A very large number of security holes are because of this precise
programming mistake.

Coverity is doing its job correctly;  it is up to a human to decide
whether we care or not.

>
>> --- a/tools/firmware/hvmloader/hvmloader.c
>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
>>  
>>      if ( !modlist ||
>>           info->modlist_paddr > UINTPTR_MAX ||
>> -         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
>> -            > UINTPTR_MAX )
>> +         (info->modlist_paddr +
>> +          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
>>          return NULL;
> This can be had without resorting to 64-bit multiplication, by bounds
> checking
>
>          (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist)
>
> instead. While we would certainly hope that compilers don't resort
> to a libgcc helper for 64-bit multiplication, I think we'd better avoid
> that risk altogether.

In this case, using libgcc would cause a link error because of
-fno-builtin, so I don't think it is too bad.  I would be surprised if
we didn't have other 64bit multiplication in hvmloader.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] hvmloader: correctly copy signature to info structures

[Jan Beulich]

>>> On 19.08.16 at 11:42, <andrew.cooper3@citrix.com> wrote:
> On 19/08/16 09:25, Jan Beulich wrote:
>>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
>>> --- a/tools/firmware/hvmloader/ovmf.c
>>> +++ b/tools/firmware/hvmloader/ovmf.c
>>> @@ -67,10 +67,11 @@ struct ovmf_info {
>>>  static void ovmf_setup_bios_info(void)
>>>  {
>>>      struct ovmf_info *info = (void *)OVMF_INFO_PHYSICAL_ADDRESS;
>>> +    const char sig[] = "XenHVMOVMF";
>>>  
>>>      memset(info, 0, sizeof(*info));
>>>  
>>> -    memcpy(info->signature, "XenHVMOVMF", sizeof(info->signature));
>>> +    memcpy(info->signature, sig, sizeof(sig));
>>>      info->length = sizeof(*info);
>>>  }
>> I think using strncpy() would be more natural in cases like this,
>> as it would at once make clear that the destination can't be
>> overrun no matter how large the string literal.
> 
> How about structure assignment?
> 
> *info = (struct ovmf_info) { .signature = "XenHVMOVMF", .length =
> sizeof(*info) }
> 
> which also subsumed the memset()?

Fine with me, albeit maybe a little uglier to read.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Jan Beulich]

>>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote:
> On 19/08/16 09:31, Jan Beulich wrote:
>>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
>>> --- a/tools/firmware/hvmloader/hvmloader.c
>>> +++ b/tools/firmware/hvmloader/hvmloader.c
>>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
>>>  
>>>      if ( !modlist ||
>>>           info->modlist_paddr > UINTPTR_MAX ||
>>> -         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
>>> -            > UINTPTR_MAX )
>>> +         (info->modlist_paddr +
>>> +          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
>>>          return NULL;
>> This can be had without resorting to 64-bit multiplication, by bounds
>> checking
>>
>>          (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist)
>>
>> instead. While we would certainly hope that compilers don't resort
>> to a libgcc helper for 64-bit multiplication, I think we'd better avoid
>> that risk altogether.
> 
> In this case, using libgcc would cause a link error because of
> -fno-builtin, so I don't think it is too bad.

And it's this link error which I want to avoid.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Wei Liu]

On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote:
> >>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote:
> > On 19/08/16 09:31, Jan Beulich wrote:
> >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
> >>> --- a/tools/firmware/hvmloader/hvmloader.c
> >>> +++ b/tools/firmware/hvmloader/hvmloader.c
> >>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
> >>>  
> >>>      if ( !modlist ||
> >>>           info->modlist_paddr > UINTPTR_MAX ||
> >>> -         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
> >>> -            > UINTPTR_MAX )
> >>> +         (info->modlist_paddr +
> >>> +          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
> >>>          return NULL;
> >> This can be had without resorting to 64-bit multiplication, by bounds
> >> checking
> >>
> >>          (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist)
> >>
> >> instead. While we would certainly hope that compilers don't resort
> >> to a libgcc helper for 64-bit multiplication, I think we'd better avoid
> >> that risk altogether.
> > 
> > In this case, using libgcc would cause a link error because of
> > -fno-builtin, so I don't think it is too bad.
> 
> And it's this link error which I want to avoid.
> 

What approach should I use? I would like to clear this minor issue as
quick as possible.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] hvmloader: cast to 64bit before multiplication in get_module_entry

[Jan Beulich]

>>> On 22.08.16 at 13:37, <wei.liu2@citrix.com> wrote:
> On Fri, Aug 19, 2016 at 06:00:12AM -0600, Jan Beulich wrote:
>> >>> On 19.08.16 at 12:09, <andrew.cooper3@citrix.com> wrote:
>> > On 19/08/16 09:31, Jan Beulich wrote:
>> >>>>> On 19.08.16 at 10:06, <wei.liu2@citrix.com> wrote:
>> >>> --- a/tools/firmware/hvmloader/hvmloader.c
>> >>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> >>> @@ -272,8 +272,8 @@ const struct hvm_modlist_entry *get_module_entry(
>> >>>  
>> >>>      if ( !modlist ||
>> >>>           info->modlist_paddr > UINTPTR_MAX ||
>> >>> -         (info->modlist_paddr + info->nr_modules * sizeof(*modlist) - 1)
>> >>> -            > UINTPTR_MAX )
>> >>> +         (info->modlist_paddr +
>> >>> +          (uint64_t)info->nr_modules * sizeof(*modlist) - 1) > UINTPTR_MAX )
>> >>>          return NULL;
>> >> This can be had without resorting to 64-bit multiplication, by bounds
>> >> checking
>> >>
>> >>          (UINTPTR_MAX - (uintptr_t)info->modlist_paddr) / sizeof(*modlist)
>> >>
>> >> instead. While we would certainly hope that compilers don't resort
>> >> to a libgcc helper for 64-bit multiplication, I think we'd better avoid
>> >> that risk altogether.
>> > 
>> > In this case, using libgcc would cause a link error because of
>> > -fno-builtin, so I don't think it is too bad.
>> 
>> And it's this link error which I want to avoid.
> 
> What approach should I use? I would like to clear this minor issue as
> quick as possible.

Well, if Andrew wants to ack the patch with the above unchanged, I
won't object. I continue to prefer the suggested alternative though.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel