Patchwork [v4,12/30] vmstate: fix buffer overflow in target-arm/machine.c

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 31, 2014, 2:16 p.m.
Message ID <1396275242-10810-13-git-send-email-mst@redhat.com>
Download mbox | patch
Permalink /patch/335395/
State New
Headers show

Comments

Michael S. Tsirkin - March 31, 2014, 2:16 p.m.
CVE-2013-4531

cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
cpreg_vmstate_array_len will cause a buffer overflow.

VMSTATE_INT32_LE was supposed to protect against this
but doesn't because it doesn't validate that input is
non-negative.

Fix this macro to valide the value appropriately.

The only other user of VMSTATE_INT32_LE doesn't
ever use negative numbers so it doesn't care.

Reported-by: Anthony Liguori <anthony@codemonkey.ws>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 vmstate.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Peter Maydell - March 31, 2014, 3:40 p.m.
On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> CVE-2013-4531
>
> cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
> cpreg_vmstate_array_len will cause a buffer overflow.
>
> VMSTATE_INT32_LE was supposed to protect against this
> but doesn't because it doesn't validate that input is
> non-negative.
>
> Fix this macro to valide the value appropriately.
>
> The only other user of VMSTATE_INT32_LE doesn't
> ever use negative numbers so it doesn't care.

This fixes the bug, but it's rather unintuitive semantics
for an INT32_LE not to simply do a signed comparison...
Maybe rename the macro?

thanks
-- PMM
Michael S. Tsirkin - April 1, 2014, 3:12 p.m.
On Mon, Mar 31, 2014 at 04:40:41PM +0100, Peter Maydell wrote:
> On 31 March 2014 15:16, Michael S. Tsirkin <mst@redhat.com> wrote:
> > CVE-2013-4531
> >
> > cpreg_vmstate_indexes is a VARRAY_INT32. A negative value for
> > cpreg_vmstate_array_len will cause a buffer overflow.
> >
> > VMSTATE_INT32_LE was supposed to protect against this
> > but doesn't because it doesn't validate that input is
> > non-negative.
> >
> > Fix this macro to valide the value appropriately.
> >
> > The only other user of VMSTATE_INT32_LE doesn't
> > ever use negative numbers so it doesn't care.
> 
> This fixes the bug, but it's rather unintuitive semantics
> for an INT32_LE not to simply do a signed comparison...
> Maybe rename the macro?
> 
> thanks
> -- PMM

Sure. I'll do it in a separate patch though.

Patch

diff --git a/vmstate.c b/vmstate.c
index 02d5d94..e1e9cae 100644
--- a/vmstate.c
+++ b/vmstate.c
@@ -340,8 +340,9 @@  const VMStateInfo vmstate_info_int32_equal = {
     .put  = put_int32,
 };
 
-/* 32 bit int. Check that the received value is less than or equal to
-   the one in the field */
+/* 32 bit int. Check that the received value is non-negative
+ * and less than or equal to the one in the field.
+ */
 
 static int get_int32_le(QEMUFile *f, void *pv, size_t size)
 {
@@ -349,7 +350,7 @@  static int get_int32_le(QEMUFile *f, void *pv, size_t size)
     int32_t loaded;
     qemu_get_sbe32s(f, &loaded);
 
-    if (loaded <= *cur) {
+    if (loaded >= 0 && loaded <= *cur) {
         *cur = loaded;
         return 0;
     }