| Message ID | 1396543778-22307-15-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03.04.2014, at 18:51, Michael S. Tsirkin <mst@redhat.com> wrote: > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > CVE-2013-4534 > > opp->nb_cpus is read from the wire and used to determine how many > IRQDest elements to read into opp->dst[]. If the value exceeds the > length of opp->dst[], MAX_CPU, opp->dst[] can be overrun with arbitrary > data from the wire. > > Fix this by failing migration if the value read from the wire exceeds > MAX_CPU. > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Alexander Graf <agraf@suse.de> Though I don't think the nb_cpus checks are necessary, it's always nice to be safe rather than sorry :). Alex > --- > hw/intc/openpic.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c > index be76fbd..e63ccf2 100644 > --- a/hw/intc/openpic.c > +++ b/hw/intc/openpic.c > @@ -1416,7 +1416,7 @@ static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q) > static int openpic_load(QEMUFile* f, void *opaque, int version_id) > { > OpenPICState *opp = (OpenPICState *)opaque; > - unsigned int i; > + unsigned int i, nb_cpus; > > if (version_id != 1) { > return -EINVAL; > @@ -1428,7 +1428,11 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id) > qemu_get_be32s(f, &opp->spve); > qemu_get_be32s(f, &opp->tfrr); > > - qemu_get_be32s(f, &opp->nb_cpus); > + qemu_get_be32s(f, &nb_cpus); > + if (opp->nb_cpus != nb_cpus) { > + return -EINVAL; > + } > + assert(nb_cpus > 0 && nb_cpus <= MAX_CPU); > > for (i = 0; i < opp->nb_cpus; i++) { > qemu_get_sbe32s(f, &opp->dst[i].ctpr); > @@ -1567,6 +1571,12 @@ static void openpic_realize(DeviceState *dev, Error **errp) > {NULL} > }; > > + if (opp->nb_cpus > MAX_CPU) { > + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, > + TYPE_OPENPIC, "nb_cpus", 0, MAX_CPU); > + return; > + } > + > switch (opp->model) { > case OPENPIC_MODEL_FSL_MPIC_20: > default: > -- > MST >
On Thu, Apr 03, 2014 at 08:04:23PM +0200, Alexander Graf wrote: > > On 03.04.2014, at 18:51, Michael S. Tsirkin <mst@redhat.com> wrote: > > > From: Michael Roth <mdroth@linux.vnet.ibm.com> > > > > CVE-2013-4534 > > > > opp->nb_cpus is read from the wire and used to determine how many > > IRQDest elements to read into opp->dst[]. If the value exceeds the > > length of opp->dst[], MAX_CPU, opp->dst[] can be overrun with arbitrary > > data from the wire. > > > > Fix this by failing migration if the value read from the wire exceeds > > MAX_CPU. > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Reviewed-by: Alexander Graf <agraf@suse.de> > > Though I don't think the nb_cpus checks are necessary, it's always nice to be safe rather than sorry :). > > > Alex > > > --- > > hw/intc/openpic.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c > > index be76fbd..e63ccf2 100644 > > --- a/hw/intc/openpic.c > > +++ b/hw/intc/openpic.c > > @@ -1416,7 +1416,7 @@ static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q) > > static int openpic_load(QEMUFile* f, void *opaque, int version_id) > > { > > OpenPICState *opp = (OpenPICState *)opaque; > > - unsigned int i; > > + unsigned int i, nb_cpus; > > > > if (version_id != 1) { > > return -EINVAL; > > @@ -1428,7 +1428,11 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id) > > qemu_get_be32s(f, &opp->spve); > > qemu_get_be32s(f, &opp->tfrr); > > > > - qemu_get_be32s(f, &opp->nb_cpus); > > + qemu_get_be32s(f, &nb_cpus); > > + if (opp->nb_cpus != nb_cpus) { > > + return -EINVAL; > > + } > > + assert(nb_cpus > 0 && nb_cpus <= MAX_CPU); > > > > for (i = 0; i < opp->nb_cpus; i++) { > > qemu_get_sbe32s(f, &opp->dst[i].ctpr); > > @@ -1567,6 +1571,12 @@ static void openpic_realize(DeviceState *dev, Error **errp) > > {NULL} > > }; > > > > + if (opp->nb_cpus > MAX_CPU) { > > + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, > > + TYPE_OPENPIC, "nb_cpus", 0, MAX_CPU); > > + return; > > + } > > + This one causes a warning on 32 bit hosts, will repost a tweaked version. > > switch (opp->model) { > > case OPENPIC_MODEL_FSL_MPIC_20: > > default: > > -- > > MST > > >
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index be76fbd..e63ccf2 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -1416,7 +1416,7 @@ static void openpic_load_IRQ_queue(QEMUFile* f, IRQQueue *q) static int openpic_load(QEMUFile* f, void *opaque, int version_id) { OpenPICState *opp = (OpenPICState *)opaque; - unsigned int i; + unsigned int i, nb_cpus; if (version_id != 1) { return -EINVAL; @@ -1428,7 +1428,11 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id) qemu_get_be32s(f, &opp->spve); qemu_get_be32s(f, &opp->tfrr); - qemu_get_be32s(f, &opp->nb_cpus); + qemu_get_be32s(f, &nb_cpus); + if (opp->nb_cpus != nb_cpus) { + return -EINVAL; + } + assert(nb_cpus > 0 && nb_cpus <= MAX_CPU); for (i = 0; i < opp->nb_cpus; i++) { qemu_get_sbe32s(f, &opp->dst[i].ctpr); @@ -1567,6 +1571,12 @@ static void openpic_realize(DeviceState *dev, Error **errp) {NULL} }; + if (opp->nb_cpus > MAX_CPU) { + error_set(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, + TYPE_OPENPIC, "nb_cpus", 0, MAX_CPU); + return; + } + switch (opp->model) { case OPENPIC_MODEL_FSL_MPIC_20: default: