[Top][All Lists]
[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.