Re: [Qemu-devel] [PATCH v4 13/30] stellaris_enet: avoid buffer overrun on incoming migration

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 06:11:22PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
> > CVE-2013-4532
> > 
> > s->next_packet is read from wire as an index into s->rx[]. If
> > s->next_packet exceeds the length of s->rx[], the buffer can be
> > subsequently overrun with arbitrary data from the wire.
> > 
> > Fix this by failing migration if s->next_packet we read from
> > the wire exceeds this.
> > 
> > Similarly, validate rx_fifo against sizeof(s->rx[].data).
> > 
> > Finally, constrain rx len to a sensibly small positive
> > value, to avoid integer overruns when data model
> > later uses this value.
> > 
> > Reported-by: Michael Roth <address@hidden>
> > Reported-by: Peter Maydell <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > ---
> >  hw/net/stellaris_enet.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> > index d04e6a4..182fd3e 100644
> > --- a/hw/net/stellaris_enet.c
> > +++ b/hw/net/stellaris_enet.c
> > @@ -358,7 +358,7 @@ static void stellaris_enet_save(QEMUFile *f, void 
> > *opaque)
> >  static int stellaris_enet_load(QEMUFile *f, void *opaque, int version_id)
> >  {
> >      stellaris_enet_state *s = (stellaris_enet_state *)opaque;
> > -    int i;
> > +    int i, v;
> >  
> >      if (version_id != 1)
> >          return -EINVAL;
> > @@ -381,9 +381,25 @@ static int stellaris_enet_load(QEMUFile *f, void 
> > *opaque, int version_id)
> >          qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> >  
> >      }
> 
> The loop that's just off the top here is:
>     for (i = 0; i < 31; i++) {
>         s->rx[i].len = qemu_get_be32(f);
>         qemu_get_buffer(f, s->rx[i].data, sizeof(s->rx[i].data));
> 
>     }
> 
> Doesn't that 'len' need validating? I assume it's the size of the
> packet in the fixed sized buffer? (??)

Not that I see where it's used as such.

> > -    s->next_packet = qemu_get_be32(f);
> > -    s->rx_fifo = s->rx[s->next_packet].data + qemu_get_be32(f);
> > -    s->rx_fifo_len = qemu_get_be32(f);
> > +    v = qemu_get_be32(f);
> > +    if (v < 0 || v >= ARRAY_SIZE(s->rx)) {
> > +        return -EINVAL;
> > +    }
> > +    s->next_packet = v;
> > +    v = qemu_get_be32(f);
> > +    if (v < 0 || v >= sizeof(s->rx[i].data)) {
> > +        return -EINVAL;
> > +    }
> > +    s->rx_fifo = s->rx[s->next_packet].data + v;
> > +    v = qemu_get_be32(f);
> > +    /* Set limit low enough to avoid integer overflow when
> > +     * we do math on len later, but high enough to avoid
> > +     * truncating any packets.
> > +     */
> > +    if (v < 0 || v >= 0x100000) {
> > +        return -EINVAL;
> > +    }
> > +    s->rx_fifo_len = v;
> 
> I don't understand this - isn't the requirement that rx_fifo+rx_fifo_len be 
> within
> the buffer (or I think it might be legal for the sum to point to the byte 
> after the
> buffer)?
> My (quick first ever look at this code) reading is that rx_fifo and 
> rx_fifo_len
> related to the current packet in-flight; although I've not quite convinced 
> myself
> about what is supposed to happen at the end of the packet (which is why
> I say rx_fifo might point just at? the end.
> 
> Dave

Actually I forgot why I wrote this last check.
Peter said we should and I thought I see the issue ...
But I no longer see what kind of damage can rx_fifo_len cause
unless validated.

I never see it used as a length - merely a counter to
detect when to increment next_packet.
And that seems to be done in a safe way ...

Peter, Dave, maybe one of you can confirm what's the
possible attack here?

> >  
> >      return 0;
> >  }
> > -- 
> > MST
> > 
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK