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

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

[Julian Seward]

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)

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

[Paul Brook]

On Tuesday 17 June 2008, Julian Seward wrote:
> +    int four = sizeof(s->regs[0]);

You're joking, right?
Either you know the value and you should use an actual number "4", or it might 
not be 4, and you should call the variable something sensible. 
Either way It should not be a modifiable variable.

Paul

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

[Robert Reif]

[-- 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)

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

[Julian Seward]

On Wednesday 18 June 2008 01:38, Robert Reif wrote:

> Here is a little cleaner version:

Yes, that is saner.

I see you removed the loop

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

Did that have some significance?  Its presence seemed very strange
to me.

J

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

[Blue Swirl]

On 6/18/08, Robert Reif <reif@earthlink.net> wrote:
> 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:
>
>
> --- 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)
>
>
>

I think a better solution would be to divide the offsets by four
(array index) and change the switch statement to match. Then the
defines could be used to remove the magic constants.