[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer o
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-stable] [Qemu-devel] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load |
Date: |
Mon, 31 Mar 2014 22:34:25 +0300 |
On Mon, Mar 31, 2014 at 07:21:30PM +0200, Laszlo Ersek wrote:
> 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")
Actually it doesn't even exist: Peter commented on this in v1
and I forgot to fix up the commit log :(
I really wanted to fix the commit log to say
"make first_multi unsigned for consistency" but
forgot.
> >
> > 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>
>
- [Qemu-stable] [PATCH v4 00/30] qemu state loading issues, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 01/30] vmstate: reduce code duplication, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 02/30] vmstate: add VMS_MUST_EXIST, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 03/30] vmstate: add VMSTATE_VALIDATE, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 04/30] virtio-net: fix buffer overflow on invalid state load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 05/30] virtio-net: out-of-bounds buffer write on load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 06/30] virtio-net: out-of-bounds buffer write on invalid state load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 07/30] virtio: out-of-bounds buffer write on invalid state load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 09/30] hpet: fix buffer overrun on invalid state load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 10/30] hw/pci/pcie_aer.c: fix buffer overruns on invalid state load, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 12/30] vmstate: fix buffer overflow in target-arm/machine.c, Michael S. Tsirkin, 2014/03/31
- [Qemu-stable] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration, Michael S. Tsirkin, 2014/03/31