From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55604) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vnx11-0006Oa-Eb for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:56:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vnx0s-0006hz-E4 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:56:19 -0500 Received: from e8.ny.us.ibm.com ([32.97.182.138]:58517) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vnx0s-0006hs-7P for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:56:10 -0500 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 3 Dec 2013 15:56:09 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <529E3510.7030101@terremark.com> References: <1386087086-3691-1-git-send-email-mst@redhat.com> <1386087086-3691-16-git-send-email-mst@redhat.com> <529E3510.7030101@terremark.com> Message-ID: <20131203205605.661.29908@loki> Date: Tue, 03 Dec 2013 14:56:05 -0600 Subject: Re: [Qemu-devel] [PATCH 15/23] pxa2xx: avoid buffer overrun on incoming migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Don Koch , "Michael S. Tsirkin" Cc: Peter Maydell , qemu-devel@nongnu.org, qemu-stable@nongnu.org Quoting Don Koch (2013-12-03 13:46:24) > On 12/03/2013 11:29 AM, Michael S. Tsirkin wrote: > > From: Michael Roth > > = > > CVE-2013-4533 > > = > > s->rx_level is read from the wire and used to determine how many bytes > > to subsequently read into s->rx_fifo[]. If s->rx_level exceeds the > > length of s->rx_fifo[] the buffer can be overrun with arbitrary data > > from the wire. > > = > > Fix this by introducing a constant, RX_FIFO_SZ, that defines the length > > of s->rx_fifo[], and taking the wire value modulo RX_FIFO_SZ (as is done > > elsewhere in the emulation code when s->rx_level exceeds RX_FIFO_SZ). > > = > > Signed-off-by: Michael Roth > > Signed-off-by: Michael S. Tsirkin > > --- > > hw/arm/pxa2xx.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > = > > diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c > > index 02b7016..41d3c39 100644 > > --- a/hw/arm/pxa2xx.c > > +++ b/hw/arm/pxa2xx.c > > @@ -457,6 +457,8 @@ static const VMStateDescription vmstate_pxa2xx_mm = =3D { > > } > > }; > > = > > +#define RX_FIFO_SZ 16 > > + > > #define TYPE_PXA2XX_SSP "pxa2xx-ssp" > > #define PXA2XX_SSP(obj) \ > > OBJECT_CHECK(PXA2xxSSPState, (obj), TYPE_PXA2XX_SSP) > > @@ -481,7 +483,7 @@ typedef struct { > > uint8_t ssrsa; > > uint8_t ssacd; > > = > > - uint32_t rx_fifo[16]; > > + uint32_t rx_fifo[RX_FIFO_SZ]; > > int rx_level; > > int rx_start; > > } PXA2xxSSPState; > > @@ -756,7 +758,7 @@ static int pxa2xx_ssp_load(QEMUFile *f, void *opaqu= e, int version_id) > > qemu_get_8s(f, &s->ssrsa); > > qemu_get_8s(f, &s->ssacd); > > = > > - s->rx_level =3D qemu_get_byte(f); > > + s->rx_level =3D qemu_get_byte(f) % RX_FIFO_SZ; > = > This looks like it could leave garbage to be read in later. Why not > check for s->rx_level > RX_FIFO_SZ and return an error like the others? When I looked at the code before it seemed like s->rx_level was a running i= ndex into a circular buffer, but I see now it never gets incremented beyond 16: if (s->enable) { uint32_t readval; readval =3D ssi_transfer(s->bus, value); if (s->rx_level < 0x10) { s->rx_fifo[(s->rx_start + s->rx_level ++) & 0xf] =3D readva= l; } else { s->sssr |=3D SSSR_ROR; } } So it probably makes more sense to just fail migration if it exceeds 16. I think that would also address the issue Peter pointed out. > = > > s->rx_start =3D 0; > > for (i =3D 0; i < s->rx_level; i ++) > > s->rx_fifo[i] =3D qemu_get_byte(f); > > = > = > -d