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

[Robert Reif <reif@earthlink.net>]

[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]

Julian Seward wrote:
> 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)
>
>
>
>   
Here is a little cleaner version:


[-- Attachment #2: eccmemctl.diff.txt --]
[-- Type: text/plain, Size: 949 bytes --]

--- hw/eccmemctl.c	(revision 4744)
+++ hw/eccmemctl.c	(working copy)
@@ -283,19 +283,16 @@
     ECCState *s = opaque;
     int i;
 
-    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;
-
-    for (i = 1; i < ECC_NREGS; i++)
-        s->regs[i] = 0;
+    s->regs[ECC_MER/4] &= (ECC_MER_VER | ECC_MER_IMPL);
+    s->regs[ECC_MER/4] |= ECC_MER_MRR;
+    s->regs[ECC_MDR/4] = 0x20;
+    s->regs[ECC_MFSR/4] = 0;
+    s->regs[ECC_VCR/4] = 0;
+    s->regs[ECC_MFAR0/4] = 0x07c00000;
+    s->regs[ECC_MFAR1/4] = 0;
+    s->regs[ECC_DR/4] = 0;
+    s->regs[ECC_ECR0/4] = 0;
+    s->regs[ECC_ECR1/4] = 0;
 }
 
 void * ecc_init(target_phys_addr_t base, qemu_irq irq, uint32_t version)