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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/08/16 09:31, Jan Beulich wrote:
>>>> On 19.08.16 at 10:06, <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel