[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on i
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on incoming migration (part 2) |
Date: |
Tue, 3 Dec 2013 20:19:35 +0000 |
On 3 December 2013 16:28, Michael S. Tsirkin <address@hidden> wrote:
> From: Michael Roth <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
> 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
- [Qemu-devel] [PATCH 16/23] virtio: validate num_sg when mapping, (continued)
- [Qemu-devel] [PATCH 16/23] virtio: validate num_sg when mapping, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 17/23] ssi-sd: fix buffer overrun on invalid state load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 18/23] ssd0323: fix buffer overun on invalid state load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 19/23] tsc210x: fix buffer overrun on invalid state load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 20/23] zaurus: fix buffer overrun on invalid state load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 21/23] usb: sanity check setup_index+setup_len in post_load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 11/23] stellaris_enet: avoid buffer overrun on incoming migration (part 2), Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 23/23] savevm: fix potential segfault on invalid state, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 22/23] virtio-scsi: fix buffer overrun on invalid state load, Michael S. Tsirkin, 2013/12/03
- [Qemu-devel] [PATCH 04/23] virtio: out-of-bounds buffer write on invalid state load, Michael S. Tsirkin, 2013/12/03
- Re: [Qemu-devel] [PATCH 00/23] qemu state loading issues, Peter Maydell, 2013/12/03