Re: [Qemu-devel] [PATCH 01/23] virtio-net: fix buffer overflow on invalid state load

[Peter Maydell]

On 3 December 2013 16:28, Michael S. Tsirkin <address@hidden> wrote:
> CVE-2013-4148 QEMU 1.0 integer conversion in
> virtio_net_load()@hw/net/virtio-net.c
>
> Deals with loading a corrupted savevm image.
>
>>         n->mac_table.in_use = qemu_get_be32(f);
>
> in_use is int so it can get negative when assigned 32bit unsigned value.
>
>>         /* MAC_TABLE_ENTRIES may be different from the saved image */
>>         if (n->mac_table.in_use <= MAC_TABLE_ENTRIES) {
>
> passing this check ^^^
>
>>             qemu_get_buffer(f, n->mac_table.macs,
>>                             n->mac_table.in_use * ETH_ALEN);
>
> with good in_use value, "n->mac_table.in_use * ETH_ALEN" can get
> positive and bigger than mac_table.macs. For example 0x81000000
> satisfies this condition when ETH_ALEN is 6.
>
> A similar problem exists with is_multi.

You mean "first_multi" (though given how the load function
sets first_multi I'm not sure how it can ever get a negative
value.)

> Fix both by making the value unsigned.

Did you audit all the uses of these fields to confirm that
making them unsigned didn't cause any issues due to
arithmetic, comparisons, etc becoming unsigned rather than
signed operations?

A safer fix would seem to be to just fix the check in the
load function to refuse negative values as well as overlarge ones.

thanks
-- PMM