[Spice-devel] [PATCH spice-common 1/3] Fix integer overflows computing sizes
Frediano Ziglio
fziglio at redhat.com
Wed Mar 21 12:03:48 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.
>
Well, from "Why not use size_t instead of uint64_t then?" is quite
hard to understand.
> 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);
> }
>
There's no such case in our protocol. Not sure if this is checked by the
Python code (I think it should).
> 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.
>
The problem is that there's no MAX_FROBS or better MAX_FROBS == UINT32_MAX.
The protocol mandate that the array is contained in the message so at the end
this patch make sure this not requiring additional protocol specification and
the behaviour is coherent between 32 and 64 bit machines.
> >
> >>>
> >>> 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()
More information about the Spice-devel
mailing list