[Qemu-devel] buffer overruns in qemu-system-sparc (hw/eccmemctl.c: ecc_reset())

[Julian Seward <jseward@acm.org>]

This is in qemu svn trunk.

Running 'qemu-system-sparc -M SS-10' (or SS-20) produces a bunch of
invalid writes in hw/eccmemctl.c: ecc_reset(), as shown at the end
of this message.  Sometimes these are sufficiently severe to corrupt
the heap and cause glibc to abort qemu.  Problem does not happen with
-M SS-5.

The writes are caused by

290:    s->regs[ECC_VCR] = 0;
291:    s->regs[ECC_MFAR0] = 0x07c00000;
292:    s->regs[ECC_MFAR1] = 0;

In fact all the assignments to s->regs[] are very strange.  regs[]
is declared with 9 elements:

  #define ECC_NREGS      9
  ...
  uint32_t regs[ECC_NREGS];

so valid indices should be 0 .. 8.  But the constants ECC_* used to
index in ..

/* Register offsets */
#define ECC_MER        0                /* Memory Enable Register */
#define ECC_MDR        4                /* Memory Delay Register */
#define ECC_MFSR       8                /* Memory Fault Status Register */
#define ECC_VCR        12               /* Video Configuration Register */
#define ECC_MFAR0      16               /* Memory Fault Address Register 0 */
#define ECC_MFAR1      20               /* Memory Fault Address Register 1 */
#define ECC_DR         24               /* Diagnostic Register */
#define ECC_ECR0       28               /* Event Count Register 0 */
#define ECC_ECR1       32               /* Event Count Register 1 */

are register offsets, not indexes; AFAICS they should be divided by
sizeof(uint32_t) before use.

Even stranger .. at eccmemctl.c:297, having (incorrectly) filled in 
s->regs[], there is a loop that zeroes all but one of the entries:

    for (i = 1; i < ECC_NREGS; i++)
        s->regs[i] = 0;

The following kludge removes the invalid writes, but is not elegant,
and still leaves the question of why the loop zeroes out the entries
afterwards.

J

Index: hw/eccmemctl.c
===================================================================
--- hw/eccmemctl.c      (revision 4744)
+++ hw/eccmemctl.c      (working copy)
@@ -281,18 +281,18 @@
 static void ecc_reset(void *opaque)
 {
     ECCState *s = opaque;
-    int i;
+    int i, four = sizeof(s->regs[0]);

-    s->regs[ECC_MER] &= (ECC_MER_VER | ECC_MER_IMPL);
-    s->regs[ECC_MER] |= ECC_MER_MRR;
-    s->regs[ECC_MDR] = 0x20;
-    s->regs[ECC_MFSR] = 0;
-    s->regs[ECC_VCR] = 0;
-    s->regs[ECC_MFAR0] = 0x07c00000;
-    s->regs[ECC_MFAR1] = 0;
-    s->regs[ECC_DR] = 0;
-    s->regs[ECC_ECR0] = 0;
-    s->regs[ECC_ECR1] = 0;
+    s->regs[ECC_MER   / four] &= (ECC_MER_VER | ECC_MER_IMPL);
+    s->regs[ECC_MER   / four] |= ECC_MER_MRR;
+    s->regs[ECC_MDR   / four] = 0x20;
+    s->regs[ECC_MFSR  / four] = 0;
+    s->regs[ECC_VCR   / four] = 0;
+    s->regs[ECC_MFAR0 / four] = 0x07c00000;
+    s->regs[ECC_MFAR1 / four] = 0;
+    s->regs[ECC_DR    / four] = 0;
+    s->regs[ECC_ECR0  / four] = 0;
+    s->regs[ECC_ECR1  / four] = 0;

     for (i = 1; i < ECC_NREGS; i++)
         s->regs[i] = 0;


The original problem:

==21509== Invalid write of size 4
==21509==    at 0x42E056: ecc_reset (eccmemctl.c:290)
==21509==    by 0x42E149: ecc_init (eccmemctl.c:323)
==21509==    by 0x425E7F: sun4m_hw_init (sun4m.c:547)
==21509==    by 0x40D50E: main (vl.c:8609)
==21509==  Address 0x7af3c20 is 8 bytes after a block of size 48 alloc'd
==21509==    at 0x4C234BB: malloc (vg_replace_malloc.c:207)
==21509==    by 0x42F995: qemu_mallocz (qemu-malloc.c:44)
==21509==    by 0x42E0DA: ecc_init (eccmemctl.c:306)
==21509==    by 0x425E7F: sun4m_hw_init (sun4m.c:547)
==21509==    by 0x40D50E: main (vl.c:8609)
==21509==
==21509== Invalid write of size 4
==21509==    at 0x42E05D: ecc_reset (eccmemctl.c:291)
==21509==    by 0x42E149: ecc_init (eccmemctl.c:323)
==21509==    by 0x425E7F: sun4m_hw_init (sun4m.c:547)
==21509==    by 0x40D50E: main (vl.c:8609)
==21509==  Address 0x7af3c30 is not stack'd, malloc'd or (recently) free'd
==21509==
==21509== Invalid write of size 4
==21509==    at 0x42E064: ecc_reset (eccmemctl.c:292)
==21509==    by 0x42E149: ecc_init (eccmemctl.c:323)
==21509==    by 0x425E7F: sun4m_hw_init (sun4m.c:547)
==21509==    by 0x40D50E: main (vl.c:8609)
==21509==  Address 0x7af3c40 is 8 bytes before a block of size 296 alloc'd
==21509==    at 0x4C234BB: malloc (vg_replace_malloc.c:207)
==21509==    by 0x40973E: register_savevm (vl.c:5958)
==21509==    by 0x42E134: ecc_init (eccmemctl.c:321)
==21509==    by 0x425E7F: sun4m_hw_init (sun4m.c:547)
==21509==    by 0x40D50E: main (vl.c:8609)