[Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes
Christophe de Dinechin
cdupontd at redhat.com
Wed Mar 21 08:18:57 UTC 2018
> On 20 Mar 2018, at 11:41, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>>
>>> On 19 Mar 2018, at 11:06, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>
>>> Make code safe using both 32 and 64 bit machine.
>>> Consider that this code can be compiled for machines with 32 bit.
>>> There are some arrays length which are 32 bit.
>>>
>>> If size_t this can cause easily an overflow. For instance message_len
>>> sending SPICE_MSG_NOTIFY messages are 32 bit and code add a small
>>> constant (currently 24) before doing the test for size. Now passing
>>> (uint32_t) -20 as message_len would lead to a size of 4 after the
>>> addition. This overflow does not happen on 64 bit machine as the length
>>> is converted to size_t.
>>
>> Why not use size_t instead of uint64_t then?
>>
>
> A multiplication between 32 bit integer and a 32 bit integer can
> cause overflow if the result is a 32 bit. Using a 64 bit integer
> to multiply a 32 bit integer by a 32 bit integer avoids the overflow.
> On 32 bit systems usually size_t is a 32 bit.
You totally missed my point then.
The problem you identified with overflow is real, and must be fixed. But it’s an input validation problem, not an arithmetic problem.
Consider:
void *allocate_frobs(uint32_t n)
{
if (n > MAX_FROBS) // input validation
return NULL;
size_t size = n * sizeof(frob) + sizeof(header);
return malloc(size);
}
This is better than:
void *allocate_frobs(uint32_t n)
{
uint64_t size = n * sizeof(frob) + sizeof(header);
if (size > UINT32_MAX)
return NULL;
return malloc(size);
}
And if you had the problem with a uint64_t as input, you would certainly not “fix” it with
void *allocate_frobs(uint64_t n)
{
double_or_uint128_t size = n * sizeof(frob) + sizeof(header);
if (size > UINT64_MAX)
return NULL;
return malloc(size);
}
Once you do the input validation on the incoming uint32_t, the rest of the computations should be done as size_t, i.e. 32-bit on a 32-bit machine.
>
>>>
>>> There are also some array length where some item are bigger than 1 byte.
>>> For instance SPICE_MAIN_CHANNELS_LIST message have a number of channels
>>> and each channel is composed by 2 bytes. Now the code generated try to do
>>> length * 2 where length is still a 32 bit so if we put a value like
>>> 0x80000002u we get 4 as length. This will cause an overflow as code will
>>> allocate very few bytes but try to fill with a huge number of elements.
>>> This overflow happen in both 32 and 64 bit machine.
>>>
>>> To avoid all these possible overflows this patch use only 64 bit for
>>> nelements (number of elements), nw_size (network size) and mem_size
>>> (memory size needed) checking the sizes to avoid other overflows
>>> (like pointers conversions under 32 bit machines).
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
>>> ---
>>> python_modules/demarshal.py | 38 +++++++++++++++++++-------------------
>>> 1 file changed, 19 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
>>> index da87d44..7e73985 100644
>>> --- a/python_modules/demarshal.py
>>> +++ b/python_modules/demarshal.py
>>> @@ -101,7 +101,7 @@ def write_parser_helpers(writer):
>>> writer.variable_def("uint64_t", "offset")
>>> writer.variable_def("parse_func_t", "parse")
>>> writer.variable_def("void **", "dest")
>>> - writer.variable_def("uint32_t", "nelements")
>>> + writer.variable_def("uint64_t", "nelements")
>>> writer.end_block(semicolon=True)
>>>
>>> def write_read_primitive(writer, start, container, name, scope):
>>> @@ -186,7 +186,7 @@ def write_validate_switch_member(writer, mprefix,
>>> container, switch_member, scop
>>>
>>> all_as_extra_size = m.is_extra_size() and want_extra_size
>>> if not want_mem_size and all_as_extra_size and not
>>> scope.variable_defined(item.mem_size()):
>>> - scope.variable_def("uint32_t", item.mem_size())
>>> + scope.variable_def("uint64_t", item.mem_size())
>>>
>>> sub_want_mem_size = want_mem_size or all_as_extra_size
>>> sub_want_extra_size = want_extra_size and not all_as_extra_size
>>> @@ -219,7 +219,7 @@ def write_validate_struct_function(writer, struct):
>>> scope = writer.function(validate_function, "static intptr_t", "uint8_t
>>> *message_start, uint8_t *message_end, uint64_t offset,
>>> SPICE_GNUC_UNUSED int minor")
>>> scope.variable_def("uint8_t *", "start = message_start + offset")
>>> scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
>>> - scope.variable_def("size_t", "mem_size", "nw_size")
>>> + scope.variable_def("uint64_t", "mem_size", "nw_size")
>>> num_pointers = struct.get_num_pointers()
>>> if num_pointers != 0:
>>> scope.variable_def("SPICE_GNUC_UNUSED intptr_t", "ptr_size")
>>> @@ -236,7 +236,7 @@ def write_validate_struct_function(writer, struct):
>>>
>>> writer.newline()
>>> writer.comment("Check if struct fits in reported side").newline()
>>> - writer.error_check("start + nw_size > message_end")
>>> + writer.error_check("nw_size > (uintptr_t) (message_end - start)")
>>>
>>> writer.statement("return mem_size")
>>>
>>> @@ -264,26 +264,26 @@ def write_validate_pointer_item(writer, container,
>>> item, scope, parent_scope, st
>>> # if array, need no function check
>>>
>>> if target_type.is_array():
>>> - writer.error_check("message_start + %s >= message_end" % v)
>>> + writer.error_check("%s >= (uintptr_t) (message_end -
>>> message_start)" % v)
>>>
>>>
>>> assert target_type.element_type.is_primitive()
>>>
>>> array_item = ItemInfo(target_type, "%s__array" % item.prefix,
>>> start)
>>> - scope.variable_def("uint32_t", array_item.nw_size())
>>> + scope.variable_def("uint64_t", array_item.nw_size())
>>> # don't create a variable that isn't used, fixes
>>> -Werror=unused-but-set-variable
>>> need_mem_size = want_mem_size or (
>>> want_extra_size and not item.member.has_attr("chunk")
>>> and not target_type.is_cstring_length())
>>> if need_mem_size:
>>> - scope.variable_def("uint32_t", array_item.mem_size())
>>> + scope.variable_def("uint64_t", array_item.mem_size())
>>> if target_type.is_cstring_length():
>>> writer.assign(array_item.nw_size(), "spice_strnlen((char
>>> *)message_start + %s, message_end - (message_start + %s))"
>>> % (v, v))
>>> writer.error_check("*(message_start + %s + %s) != 0" % (v,
>>> array_item.nw_size()))
>>> else:
>>> write_validate_array_item(writer, container, array_item,
>>> scope, parent_scope, start,
>>> True,
>>> want_mem_size=need_mem_size,
>>> want_extra_size=False)
>>> - writer.error_check("message_start + %s + %s > message_end"
>>> % (v, array_item.nw_size()))
>>> + writer.error_check("%s + %s > (uintptr_t) (message_end -
>>> message_start)" % (v, array_item.nw_size()))
>>>
>>> if want_extra_size:
>>> if item.member and item.member.has_attr("chunk"):
>>> @@ -321,11 +321,11 @@ def write_validate_array_item(writer, container,
>>> item, scope, parent_scope, star
>>> nelements = "%s__nbytes" %(item.prefix)
>>> real_nelements = "%s__nelements" %(item.prefix)
>>> if not parent_scope.variable_defined(real_nelements):
>>> - parent_scope.variable_def("uint32_t", real_nelements)
>>> + parent_scope.variable_def("uint64_t", real_nelements)
>>> else:
>>> nelements = "%s__nelements" %(item.prefix)
>>> if not parent_scope.variable_defined(nelements):
>>> - parent_scope.variable_def("uint32_t", nelements)
>>> + parent_scope.variable_def("uint64_t", nelements)
>>>
>>> if array.is_constant_length():
>>> writer.assign(nelements, array.size)
>>> @@ -420,10 +420,10 @@ def write_validate_array_item(writer, container,
>>> item, scope, parent_scope, star
>>> element_nw_size = element_item.nw_size()
>>> element_mem_size = element_item.mem_size()
>>> element_extra_size = element_item.extra_size()
>>> - scope.variable_def("uint32_t", element_nw_size)
>>> - scope.variable_def("uint32_t", element_mem_size)
>>> + scope.variable_def("uint64_t", element_nw_size)
>>> + scope.variable_def("uint64_t", element_mem_size)
>>> if want_extra_size:
>>> - scope.variable_def("uint32_t", element_extra_size)
>>> + scope.variable_def("uint64_t", element_extra_size)
>>>
>>> if want_nw_size:
>>> writer.assign(nw_size, 0)
>>> @@ -556,7 +556,7 @@ def write_validate_container(writer, prefix, container,
>>> start, parent_scope, wan
>>> sub_want_nw_size = want_nw_size and not m.is_fixed_nw_size()
>>> sub_want_mem_size = m.is_extra_size() and want_mem_size
>>> sub_want_extra_size = not m.is_extra_size() and
>>> m.contains_extra_size()
>>> - defs = ["size_t"]
>>> + defs = ["uint64_t"]
>>> name = prefix_m(prefix, m)
>>> if sub_want_nw_size:
>>>
>>> @@ -697,7 +697,7 @@ def read_array_len(writer, prefix, array, dest, scope,
>>> is_ptr):
>>> if dest.is_toplevel() and scope.variable_defined(nelements):
>>> return nelements # Already there for toplevel, need not recalculate
>>> element_type = array.element_type
>>> - scope.variable_def("uint32_t", nelements)
>>> + scope.variable_def("uint64_t", nelements)
>>> if array.is_constant_length():
>>> writer.assign(nelements, array.size)
>>> elif array.is_identifier_length():
>>> @@ -1053,9 +1053,9 @@ def write_msg_parser(writer, message):
>>> parent_scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
>>> parent_scope.variable_def("uint8_t *", "start = message_start")
>>> parent_scope.variable_def("uint8_t *", "data = NULL")
>>> - parent_scope.variable_def("size_t", "nw_size")
>>> + parent_scope.variable_def("uint64_t", "nw_size")
>>> if want_mem_size:
>>> - parent_scope.variable_def("size_t", "mem_size")
>>> + parent_scope.variable_def("uint64_t", "mem_size")
>>> if not message.has_attr("nocopy"):
>>> parent_scope.variable_def("uint8_t *", "in", "end")
>>> num_pointers = message.get_num_pointers()
>>> @@ -1073,7 +1073,7 @@ def write_msg_parser(writer, message):
>>> writer.newline()
>>>
>>> writer.comment("Check if message fits in reported side").newline()
>>> - with writer.block("if (start + nw_size > message_end)"):
>>> + with writer.block("if (nw_size > (uintptr_t) (message_end - start))"):
>>> writer.statement("return NULL")
>>>
>>> writer.newline().comment("Validated extents and calculated
>>> size").newline()
>>> @@ -1084,7 +1084,7 @@ def write_msg_parser(writer, message):
>>> writer.assign("*size", "message_end - message_start")
>>> writer.assign("*free_message", "nofree")
>>> else:
>>> - writer.assign("data", "(uint8_t *)malloc(mem_size)")
>>> + writer.assign("data", "(uint8_t *)(mem_size > UINT32_MAX ? NULL :
>>> malloc(mem_size))")
>>> writer.error_check("data == NULL")
>>> writer.assign("end", "data + %s" % (msg_sizeof))
>>> writer.assign("in", "start").newline()
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list