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

[Michael S. Tsirkin]

On Mon, Mar 31, 2014 at 10:13:04PM +0100, Peter Maydell wrote:
> On 31 March 2014 21:49, Michael S. Tsirkin <address@hidden> wrote:
> > 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.
> 
> In the DATA case of stellaris_enet_read() -- when the current
> rx_fifo_len goes to zero we will uncritically set rx_fifo_len to
> s->rx[s->next_packet].len. So we must validate that it's between
> 0 and 2048 (the size of the rx[].data array), otherwise further
> reads from DATA will be able to run off the end of the data array
> for the following packet.
> 
> >> > -    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.
> 
> > 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.
> 
> Again, look at the DATA read logic. Every time the guest does a
> DATA read, we read from the four bytes at s->rx_fifo, increment
> rx_fifo by 4 and decrement rx_fifo_len by 4. When rx_fifo_len
> eventually goes to zero we will (on the subsequent read) reset
> both rx_fifo and rx_fifo_len from the next packet in the rx queue.
> So if the incoming data sets rx_fifo_len to (let us say) 0x10000,
> then the guest can cause us to read well off the end of the rx data
> array. This means your check isn't tight enough -- we need to
> ensure that rx_fifo and rx_fifo_len between them define a window
> into the rx data and nowhere else. As David says this means you
> need:
> 
>     v1 = qemu_get_be32(f);
>     if (v1 < 0 || v1 > sizeof(s->rx[i].data)) {
>         return -EINVAL;
>     }
>     s->rx_fifo = s->rx[s->next_packet].data + v1;
>     v2 = qemu_get_be32(f);
>     if (v2 < 0 || v1 + v2 > sizeof(s->rx[i].data)) {
>         return -EINVAL;
>     }
>     s->rx_fifo_len = v2;
> 
> The max check on v1 is actually only there to ensure that we
> don't have to think about integer overflow when we do the
> upper-bound check on v1 + v2. Note that v1 == sizeof(array)
> is OK if (and only if) v2 == 0.
> 
> An assert in stellaris_enet_receive() that the net code never
> hands us a packet we can't fit in the array wouldn't go amiss
> either, but that's a separate issue.
> 
> thanks
> -- PMM


Got it now, thanks a lot for the explanation.