qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Blue Swirl
Subject: Re: [Qemu-devel] buffer overruns in qemu-system-sparc (hw/eccmemctl.c: ecc_reset())
Date: Thu, 19 Jun 2008 17:11:38 +0300

On 6/18/08, Robert Reif <address@hidden> 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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]