Re: [Qemu-devel] [PATCH v4 14/30] stellaris_enet: avoid buffer overrun on incoming migration (part 2)

[Peter Maydell]

On 1 April 2014 10:43, Dr. David Alan Gilbert <address@hidden> wrote:
> * Michael S. Tsirkin (address@hidden) wrote:
>> CVE-2013-4532
>> @@ -374,7 +374,13 @@ static int stellaris_enet_load(QEMUFile *f, void 
>> *opaque, int version_id)
>>      s->mrxd = qemu_get_be32(f);
>>      s->np = qemu_get_be32(f);
>>      s->tx_frame_len = qemu_get_be32(f);
>> -    s->tx_fifo_len = qemu_get_be32(f);
>> +    v = qemu_get_be32(f);
>> +    /* How many bytes does data use in tx fifo. */
>> +    sz = s->tx_frame_len == -1 ? 2 : 4;
>> +    if (v < 0 || v >= ARRAY_SIZE(s->tx_fifo) - sz) {
>> +        return -EINVAL;
>> +    }
>> +    s->tx_fifo_len = v;
>>      qemu_get_buffer(f, s->tx_fifo, sizeof(s->tx_fifo));
>
> I don't understand the magic '2' in that ?2:4 - but there again the 'DATA' 
> write
> case is pretty hairy.

It's not that complicated. The first word of DATA has
the frame length in the bottom 16 bits, and the first
16 bits of actual data. All the subsequent words written
to DATA are part of the actual data.

However, I think this checking code is wrong. We
need to check tx_frame_len as well -- otherwise the
incoming data can simply set a hugely oversize
frame length and then have the guest stuff data
into DATA until we overrun the tx_fifo. As with the
rx fifo, I think the right approach here is to actually
check that the incoming migration data satisfies the
invariants:
 tx_frame_len == -1
OR
 tx_frame_len >= 0 && tx_frame_len <= ARRAY_SIZE(tx_fifo)
  && tx_fifo_len >= 0 && tx_fifo_len <= tx_frame_len - 4

(We don't have to have funny ?2:4 logic because the
case where we only write 2 bytes is when tx_frame_len
is -1, which will also reinitialize tx_fifo_len and
write strictly to the start of the fifo.)

But note that there seems to be a bug or two in
the DATA read logic: our cutoff for tx frame too
long is tx_frame_len > 2032, but for the limit
case of 2032, if we add 14 for the ethernet header
and 4 for explicit CRC then we get 2050, which is
slightly larger than the tx_fifo buffer... I think
we either need to wind that 2032 value down or
enlarge the tx_fifo buffer, or possibly just tweak
the logic not to try to actually write the CRC bytes
into the FIFO since we promptly ignore them -- need
to check the h/w datasheet to find out which.

Also a bug I think is that the PADEN handling code
is setting tx_fifo_len to 60 for short packets, which
will have no effect; I'm pretty sure it should be
setting tx_frame_len instead. That's just wrong
behaviour rather than an overrun issue though.

thanks
-- PMM