On 3 December 2013 16:28, Michael S. Tsirkin <m...@redhat.com> wrote:
> From: Michael Roth <mdr...@linux.vnet.ibm.com>
>
> CVE-2013-4532
>
> s->tx_fifo_len is read from the wire and later used as an index into
> s->tx_fifo[] when a DATA command is issued by the guest. If
> s->tx_fifo_len is greater than the length of s->tx_fifo[], or less
> than 0, the buffer can be overrun/underrun by arbitrary data written out
> by the guest upon resuming it's execution.
>
> Fix this by introducing a constant that defines the length of
> s->tx_fifo[] and failing migration if the value from the wire exceeds
> this, or is less than 0.
>
> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
> ---
> hw/net/stellaris_enet.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index db12a99..65f0ba8 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -63,10 +63,11 @@ typedef struct {
> int tx_fifo_len;
> /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
> We implement a full 31 packet fifo. */
> - uint8_t tx_fifo[2048];
> +#define SE_FIFO_LEN 2048
> + uint8_t tx_fifo[SE_FIFO_LEN];
> #define SE_RX_BUF_LEN 31
> struct {
> - uint8_t data[2048];
> + uint8_t data[SE_FIFO_LEN];
> int len;
> } rx[SE_RX_BUF_LEN];
> uint8_t *rx_fifo;
> @@ -375,6 +376,9 @@ static int stellaris_enet_load(QEMUFile *f, void *opaque,
> int version_id)
> s->np = qemu_get_be32(f);
> s->tx_frame_len = qemu_get_be32(f);
> s->tx_fifo_len = qemu_get_be32(f);
> + if (s->tx_fifo_len < 0 || s->tx_fifo_len > SE_FIFO_LEN) {
> + return -EINVAL;
> + }
Actually this isn't quite right, is it? tx_fifo_len should only be
allowed to be SE_FIFO_LEN-3 .. SE_FIFO_LEN if tx_frame_len
is -1. Otherwise the guest can write up to 4 bytes off the end of
the array. (Admittedly that's pretty harmless as it's just into the
rx fifo at the moment, but still.)
thanks
-- PMM
