Re: [Qemu-stable] [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load

[Laszlo Ersek]

On 03/31/14 16:16, Michael S. Tsirkin 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.

("first_multi")

> 
> Fix both by making the value unsigned.
> 
> Reviewed-by: Michael Roth <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> Edit: "for consistency, change first_multi as well".
> 
> Note: all call sites were audited to confirm that
> making them unsigned didn't cause any issues:
> it turns out we actually never do math on them,
> so it's easy to validate because both values are
> always <= MAC_TABLE_ENTRIES.
> ---
>  include/hw/virtio/virtio-net.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index df60f16..4b32440 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -176,8 +176,8 @@ typedef struct VirtIONet {
>      uint8_t nobcast;
>      uint8_t vhost_started;
>      struct {
> -        int in_use;
> -        int first_multi;
> +        uint32_t in_use;
> +        uint32_t first_multi;
>          uint8_t multi_overflow;
>          uint8_t uni_overflow;
>          uint8_t *macs;
> 

I ran

  git grep -EHn '\<(in_use|first_multi)\>'

Many hits, hard to audit (esp. because I'm unfamiliar with the code).
Several loops with signed int loop variables. I checked cursorily.

Reviewed-by: Laszlo Ersek <address@hidden>