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

["Blue Swirl" <blauwirbel@gmail.com>]

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.