| Message ID | 1386087086-3691-8-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: > 4) CVE-2013-4529 > hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is > too large > > There are two issues in this file: > 1. log_max from remote can be larger than on local > then buffer will overrun with data coming from state file. > 2. log_num can be larger then we get data corrution > again with an overflow but not adversary controlled. > > Fix both issues. > > Reported-by: Anthony Liguori <anthony@codemonkey.ws> > Reported-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/pci/pcie_aer.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c > index 991502e..92f3f20 100644 > --- a/hw/pci/pcie_aer.c > +++ b/hw/pci/pcie_aer.c > @@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = { > } > }; > > +static int pcie_aer_state_post_load(void *opaque, int version_id) > +{ > + PCIEAERLog *s = opaque; > + > + if (s->log_num > s->log_max) { > + return -1; > + } > + return 0; > +} > + > const VMStateDescription vmstate_pcie_aer_log = { > .name = "PCIE_AER_ERROR_LOG", > .version_id = 1, > .minimum_version_id = 1, > .minimum_version_id_old = 1, > + .post_load = pcie_aer_state_post_load, > .fields = (VMStateField[]) { > VMSTATE_UINT16(log_num, PCIEAERLog), > - VMSTATE_UINT16(log_max, PCIEAERLog), > - VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, > + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), > + VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max, > vmstate_pcie_aer_err, PCIEAERErr), > VMSTATE_END_OF_LIST() > } Isn't this a migration compability break? If so the version ID needs to be bumped. thanks -- PMM
On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote: > On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: > > 4) CVE-2013-4529 > > hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is > > too large > > > > There are two issues in this file: > > 1. log_max from remote can be larger than on local > > then buffer will overrun with data coming from state file. > > 2. log_num can be larger then we get data corrution > > again with an overflow but not adversary controlled. > > > > Fix both issues. > > > > Reported-by: Anthony Liguori <anthony@codemonkey.ws> > > Reported-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > hw/pci/pcie_aer.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c > > index 991502e..92f3f20 100644 > > --- a/hw/pci/pcie_aer.c > > +++ b/hw/pci/pcie_aer.c > > @@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = { > > } > > }; > > > > +static int pcie_aer_state_post_load(void *opaque, int version_id) > > +{ > > + PCIEAERLog *s = opaque; > > + > > + if (s->log_num > s->log_max) { > > + return -1; > > + } > > + return 0; > > +} > > + > > const VMStateDescription vmstate_pcie_aer_log = { > > .name = "PCIE_AER_ERROR_LOG", > > .version_id = 1, > > .minimum_version_id = 1, > > .minimum_version_id_old = 1, > > + .post_load = pcie_aer_state_post_load, > > .fields = (VMStateField[]) { > > VMSTATE_UINT16(log_num, PCIEAERLog), > > - VMSTATE_UINT16(log_max, PCIEAERLog), > > - VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, > > + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), > > + VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max, > > vmstate_pcie_aer_err, PCIEAERErr), > > VMSTATE_END_OF_LIST() > > } > > Isn't this a migration compability break? How is it a break? > If so the version ID > needs to be bumped. > > thanks > -- PMM
On 3 December 2013 20:41, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote: >> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: >> > const VMStateDescription vmstate_pcie_aer_log = { >> > .name = "PCIE_AER_ERROR_LOG", >> > .version_id = 1, >> > .minimum_version_id = 1, >> > .minimum_version_id_old = 1, >> > + .post_load = pcie_aer_state_post_load, >> > .fields = (VMStateField[]) { >> > VMSTATE_UINT16(log_num, PCIEAERLog), >> > - VMSTATE_UINT16(log_max, PCIEAERLog), >> > - VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, >> > + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), >> > + VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max, >> > vmstate_pcie_aer_err, PCIEAERErr), >> > VMSTATE_END_OF_LIST() >> > } >> >> Isn't this a migration compability break? > > How is it a break? If a QEMU with this patch sends data to a QEMU without it, then the receiving end will think it should expect log_num array entries but the sending end is going to send log_max of them. Conversely, an old->new migration is going to send fewer array entries than the destination expects. Or have I misinterpreted how the VARRAY entries work? thanks -- PMM
On 12/03/2013 01:59 PM, Peter Maydell wrote: > > If a QEMU with this patch sends data to a QEMU without it, then the > receiving end will think it should expect log_num array entries but the > sending end is going to send log_max of them. Conversely, an old->new > migration is going to send fewer array entries than the destination > expects. Or have I misinterpreted how the VARRAY entries work? If a qemu sends data larger than the field, the source side is already compromised. All we care about is that the destination fails gracefully, rather than acting on the bogus information from the compromised source. Versioning is only necessary for correct migration data, and doesn't matter when the source is already compromised.
On 3 December 2013 21:19, Eric Blake <eblake@redhat.com> wrote: > On 12/03/2013 01:59 PM, Peter Maydell wrote: > >> >> If a QEMU with this patch sends data to a QEMU without it, then the >> receiving end will think it should expect log_num array entries but the >> sending end is going to send log_max of them. Conversely, an old->new >> migration is going to send fewer array entries than the destination >> expects. Or have I misinterpreted how the VARRAY entries work? > > If a qemu sends data larger than the field, the source side is already > compromised. Not if the reason it's sending data larger than the field is because it's a non-compromised QEMU with this patch which makes it send log_max entries regardless of log_num, surely? -- PMM
On Tue, Dec 03, 2013 at 08:59:52PM +0000, Peter Maydell wrote: > On 3 December 2013 20:41, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Dec 03, 2013 at 06:30:46PM +0000, Peter Maydell wrote: > >> On 3 December 2013 16:28, Michael S. Tsirkin <mst@redhat.com> wrote: > >> > const VMStateDescription vmstate_pcie_aer_log = { > >> > .name = "PCIE_AER_ERROR_LOG", > >> > .version_id = 1, > >> > .minimum_version_id = 1, > >> > .minimum_version_id_old = 1, > >> > + .post_load = pcie_aer_state_post_load, > >> > .fields = (VMStateField[]) { > >> > VMSTATE_UINT16(log_num, PCIEAERLog), > >> > - VMSTATE_UINT16(log_max, PCIEAERLog), > >> > - VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, > >> > + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), > >> > + VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max, > >> > vmstate_pcie_aer_err, PCIEAERErr), > >> > VMSTATE_END_OF_LIST() > >> > } > >> > >> Isn't this a migration compability break? > > > > How is it a break? > > If a QEMU with this patch sends data to a QEMU without it, then the > receiving end will think it should expect log_num array entries but the > sending end is going to send log_max of them. Conversely, an old->new > migration is going to send fewer array entries than the destination > expects. Or have I misinterpreted how the VARRAY entries work? > > thanks > -- PMM Ah, got it. You are right, good catch. I think we need VMSTATE_UINT16_LE, this will make sure log_num <= log_max without changing VMSTATE_STRUCT_VARRAY size.
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index 991502e..92f3f20 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -795,15 +795,26 @@ static const VMStateDescription vmstate_pcie_aer_err = { } }; +static int pcie_aer_state_post_load(void *opaque, int version_id) +{ + PCIEAERLog *s = opaque; + + if (s->log_num > s->log_max) { + return -1; + } + return 0; +} + const VMStateDescription vmstate_pcie_aer_log = { .name = "PCIE_AER_ERROR_LOG", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, + .post_load = pcie_aer_state_post_load, .fields = (VMStateField[]) { VMSTATE_UINT16(log_num, PCIEAERLog), - VMSTATE_UINT16(log_max, PCIEAERLog), - VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_num, + VMSTATE_UINT16_EQUAL(log_max, PCIEAERLog), + VMSTATE_STRUCT_VARRAY_POINTER_UINT16(log, PCIEAERLog, log_max, vmstate_pcie_aer_err, PCIEAERErr), VMSTATE_END_OF_LIST() }
4) CVE-2013-4529 hw/pci/pcie_aer.c pcie aer log can overrun the buffer if log_num is too large There are two issues in this file: 1. log_max from remote can be larger than on local then buffer will overrun with data coming from state file. 2. log_num can be larger then we get data corrution again with an overflow but not adversary controlled. Fix both issues. Reported-by: Anthony Liguori <anthony@codemonkey.ws> Reported-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/pci/pcie_aer.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)