From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K9KrU-0006gh-Ph for qemu-devel@nongnu.org; Thu, 19 Jun 2008 10:11:40 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K9KrU-0006gT-4M for qemu-devel@nongnu.org; Thu, 19 Jun 2008 10:11:40 -0400 Received: from [199.232.76.173] (port=44922 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K9KrT-0006gQ-Vy for qemu-devel@nongnu.org; Thu, 19 Jun 2008 10:11:40 -0400 Received: from wx-out-0506.google.com ([66.249.82.229]:30076) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K9KrT-00072P-NP for qemu-devel@nongnu.org; Thu, 19 Jun 2008 10:11:39 -0400 Received: by wx-out-0506.google.com with SMTP id h29so177543wxd.4 for ; Thu, 19 Jun 2008 07:11:38 -0700 (PDT) Message-ID: Date: Thu, 19 Jun 2008 17:11:38 +0300 From: "Blue Swirl" Subject: Re: [Qemu-devel] buffer overruns in qemu-system-sparc (hw/eccmemctl.c: ecc_reset()) In-Reply-To: <48584B13.7000503@earthlink.net> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200806172235.19974.jseward@acm.org> <48584B13.7000503@earthlink.net> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On 6/18/08, Robert Reif 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.