Message ID | 1386087086-3691-16-git-send-email-mst@redhat.com |
---|---|
State | New |
Headers | show |
On 12/03/2013 11:29 AM, Michael S. Tsirkin wrote: > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > 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 <mdroth@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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 = { > } > }; > > +#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 *opaque, int version_id) > qemu_get_8s(f, &s->ssrsa); > qemu_get_8s(f, &s->ssacd); > > - s->rx_level = qemu_get_byte(f); > + s->rx_level = 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? > s->rx_start = 0; > for (i = 0; i < s->rx_level; i ++) > s->rx_fifo[i] = qemu_get_byte(f); > -d
On 3 December 2013 16:29, Michael S. Tsirkin <mst@redhat.com> wrote: > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > 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 <mdroth@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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 = { > } > }; > > +#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 *opaque, int version_id) > qemu_get_8s(f, &s->ssrsa); > qemu_get_8s(f, &s->ssacd); > > - s->rx_level = qemu_get_byte(f); > + s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ; > s->rx_start = 0; > for (i = 0; i < s->rx_level; i ++) > s->rx_fifo[i] = qemu_get_byte(f); An rx_level of 16 is OK, but this change will incorrectly read it as zero, won't it? thanks -- PMM
Quoting Don Koch (2013-12-03 13:46:24) > On 12/03/2013 11:29 AM, Michael S. Tsirkin wrote: > > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > > > 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 <mdroth@linux.vnet.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > 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 = { > > } > > }; > > > > +#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 *opaque, int version_id) > > qemu_get_8s(f, &s->ssrsa); > > qemu_get_8s(f, &s->ssacd); > > > > - s->rx_level = qemu_get_byte(f); > > + s->rx_level = 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 index into a circular buffer, but I see now it never gets incremented beyond 16: if (s->enable) { uint32_t readval; readval = ssi_transfer(s->bus, value); if (s->rx_level < 0x10) { s->rx_fifo[(s->rx_start + s->rx_level ++) & 0xf] = readval; } else { s->sssr |= 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 = 0; > > for (i = 0; i < s->rx_level; i ++) > > s->rx_fifo[i] = qemu_get_byte(f); > > > > -d
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 = { } }; +#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 *opaque, int version_id) qemu_get_8s(f, &s->ssrsa); qemu_get_8s(f, &s->ssacd); - s->rx_level = qemu_get_byte(f); + s->rx_level = qemu_get_byte(f) % RX_FIFO_SZ; s->rx_start = 0; for (i = 0; i < s->rx_level; i ++) s->rx_fifo[i] = qemu_get_byte(f);