From patchwork Fri Mar 20 00:02:32 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nolan X-Patchwork-Id: 13186 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n2K02cpe001109 for ; Fri, 20 Mar 2009 00:02:38 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751767AbZCTACg (ORCPT ); Thu, 19 Mar 2009 20:02:36 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751892AbZCTACf (ORCPT ); Thu, 19 Mar 2009 20:02:35 -0400 Received: from phong.sigbus.net ([65.49.35.42]:53560 "EHLO phong.sigbus.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751670AbZCTACf (ORCPT ); Thu, 19 Mar 2009 20:02:35 -0400 Received: from [192.168.0.3] (c-71-202-202-194.hsd1.ca.comcast.net [71.202.202.194]) by phong.sigbus.net (Postfix) with ESMTPSA id E9A8B95C06C; Thu, 19 Mar 2009 17:02:32 -0700 (PDT) Subject: Re: [PATCH] fix extboot from boot with cache=off From: Nolan To: Avi Kivity Cc: kvm@vger.kernel.org In-Reply-To: <49C21860.50302@redhat.com> References: <1237258333.15350.62.camel@voxel> <49C21860.50302@redhat.com> Date: Thu, 19 Mar 2009 17:02:32 -0700 Message-Id: <1237507352.15350.145.camel@voxel> Mime-Version: 1.0 X-Mailer: Evolution 2.24.3 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Thanks for the prompt review! On Thu, 2009-03-19 at 12:03 +0200, Avi Kivity wrote: > Nolan wrote: > > if (cmd->type == 0x01 || cmd->type == 0x02) { > > - target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment; > > + pa = cmd->xfer.segment * 16 + cmd->xfer.offset; > > + blen = cmd->xfer.nb_sectors * 512; > > + buf = qemu_memalign(512, blen); > > + if (!buf) { > > + printf("qemu_memalign failed\n"); > > + return; > > + } > > Don't check for allocation failures. qemu_malloc() terminates > gracefully on failure, qemu_memalign() does not, but should. OK, I've removed the check. As long as I'm in here, I've removed the check of the return value of qemu_mallocz in extboot_init as well. I'll send a separate patch to qemu-devel with the change to make qemu_memalign abort on failure. > > /* possible buffer overflow */ > > - if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size) > > + if ((pa + blen) > phys_ram_size) { > > + qemu_free(buf); > > return; > > + } > > > > cpu_physical_memory_rw() will check these for you. Check removed. New patch appended: Signed-off-by: Nolan Leake sigbus.net> --- qemu/hw/extboot.c | 35 +++++++++++++++++------------------ 1 files changed, 17 insertions(+), 18 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/qemu/hw/extboot.c b/qemu/hw/extboot.c index ada0fdd..32e6226 100644 --- a/qemu/hw/extboot.c +++ b/qemu/hw/extboot.c @@ -77,19 +77,19 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value) BlockDriverState *bs = opaque; int cylinders, heads, sectors, err; uint64_t nb_sectors; - - get_translated_chs(bs, &cylinders, &heads, §ors); + target_phys_addr_t pa; + int blen; + void *buf = NULL; if (cmd->type == 0x01 || cmd->type == 0x02) { - target_ulong pa = cmd->xfer.segment * 16 + cmd->xfer.segment; - - /* possible buffer overflow */ - if ((pa + cmd->xfer.nb_sectors * 512) > phys_ram_size) - return; + pa = cmd->xfer.segment * 16 + cmd->xfer.offset; + blen = cmd->xfer.nb_sectors * 512; + buf = qemu_memalign(512, blen); } switch (cmd->type) { case 0x00: + get_translated_chs(bs, &cylinders, &heads, §ors); bdrv_get_geometry(bs, &nb_sectors); cmd->query_geometry.cylinders = cylinders; cmd->query_geometry.heads = heads; @@ -98,22 +98,25 @@ static void extboot_write_cmd(void *opaque, uint32_t addr, uint32_t value) cpu_physical_memory_set_dirty((value & 0xFFFF) << 4); break; case 0x01: - err = bdrv_read(bs, cmd->xfer.sector, phys_ram_base + - cmd->xfer.segment * 16 + cmd->xfer.offset, - cmd->xfer.nb_sectors); + err = bdrv_read(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors); if (err) printf("Read failed\n"); + + cpu_physical_memory_write(pa, buf, blen); + break; case 0x02: - err = bdrv_write(bs, cmd->xfer.sector, phys_ram_base + - cmd->xfer.segment * 16 + cmd->xfer.offset, - cmd->xfer.nb_sectors); + cpu_physical_memory_read(pa, buf, blen); + + err = bdrv_write(bs, cmd->xfer.sector, buf, cmd->xfer.nb_sectors); if (err) printf("Write failed\n"); - cpu_physical_memory_set_dirty(cmd->xfer.segment * 16 + cmd->xfer.offset); break; } + + if (buf) + qemu_free(buf); } void extboot_init(BlockDriverState *bs, int cmd) @@ -121,10 +124,6 @@ void extboot_init(BlockDriverState *bs, int cmd) int *pcmd; pcmd = qemu_mallocz(sizeof(int)); - if (!pcmd) { - fprintf(stderr, "Error allocating memory\n"); - exit(1); - } *pcmd = cmd; register_ioport_read(0x404, 1, 1, extboot_read, pcmd);