> On 19 Mar 2018, at 11:06, Frediano Ziglio <fziglio@xxxxxxxxxx> 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?
>
> 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@xxxxxxxxxx>
> ---
> 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()
> --
> 2.14.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel