From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K8klS-0002bH-7u for qemu-devel@nongnu.org; Tue, 17 Jun 2008 19:39:02 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K8klR-0002b1-Rm for qemu-devel@nongnu.org; Tue, 17 Jun 2008 19:39:01 -0400 Received: from [199.232.76.173] (port=37632 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K8klR-0002ay-II for qemu-devel@nongnu.org; Tue, 17 Jun 2008 19:39:01 -0400 Received: from pop-knobcone.atl.sa.earthlink.net ([207.69.195.64]:38849) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K8klR-000816-K4 for qemu-devel@nongnu.org; Tue, 17 Jun 2008 19:39:01 -0400 Received: from user-142h2k8.cable.mindspring.com ([72.40.138.136] helo=[192.168.0.90]) by pop-knobcone.atl.sa.earthlink.net with esmtp (Exim 3.36 #1) id 1K8klQ-0003OL-00 for qemu-devel@nongnu.org; Tue, 17 Jun 2008 19:39:00 -0400 Message-ID: <48584B13.7000503@earthlink.net> Date: Tue, 17 Jun 2008 19:38:59 -0400 From: Robert Reif MIME-Version: 1.0 Subject: Re: [Qemu-devel] buffer overruns in qemu-system-sparc (hw/eccmemctl.c: ecc_reset()) References: <200806172235.19974.jseward@acm.org> In-Reply-To: <200806172235.19974.jseward@acm.org> Content-Type: multipart/mixed; boundary="------------060504000102070700060002" 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 This is a multi-part message in MIME format. --------------060504000102070700060002 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit 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: --------------060504000102070700060002 Content-Type: text/plain; name="eccmemctl.diff.txt" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="eccmemctl.diff.txt" --- 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) --------------060504000102070700060002--