From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K0DIB-0005Ap-FX for qemu-devel@nongnu.org; Sun, 25 May 2008 06:17:31 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K0DI8-00056z-Qm for qemu-devel@nongnu.org; Sun, 25 May 2008 06:17:30 -0400 Received: from [199.232.76.173] (port=42022 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K0DI8-00056a-ED for qemu-devel@nongnu.org; Sun, 25 May 2008 06:17:28 -0400 Received: from tapir.sajinet.com.pe ([66.139.79.212]:35698) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K0DI8-0005Pn-2o for qemu-devel@nongnu.org; Sun, 25 May 2008 06:17:28 -0400 Date: Sun, 25 May 2008 05:38:38 -0500 From: Carlo Marcelo Arenas Belon Subject: Re: [Qemu-devel] [PATCH] Fix ATAPI GET_CONFIGURATION function Message-ID: <20080525103837.GA16584@tapir> References: <48343300.7050902@suse.de> <51775D7B-EA5B-471C-A664-ADA734850CB6@suse.de> <20080525075319.GA2627@tapir> <3D336F70-E1E2-410A-BA62-ED3F6295DD09@suse.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="d6Gm4EdcadzBjdND" Content-Disposition: inline In-Reply-To: <3D336F70-E1E2-410A-BA62-ED3F6295DD09@suse.de> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: qemu-devel@nongnu.org, Paul Brook --d6Gm4EdcadzBjdND Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Alexander sorry if I wasn't clear enough. I wasn't arguing about your patch (which is correct) but was trying to explain why the old implementation was done the way it was, in an (obviously excessive) effort to prevent a buffer overflow for the response (which used to be hardcoded to 32 bytes regardless of the size of the buffer) as shown by : http://svn.savannah.gnu.org/viewvc/trunk/hw/ide.c?root=qemu&r1=3147&r2=3161 a slightly modified version of your patch (which I'd been using against kvm and validated correct with Linux and Solaris guests) attached. the only difference, is that it handles explicitly the empty buffer case and cleans up the len calculation which shouldn't had been calculated conditionally as you pointed out. Carlo --d6Gm4EdcadzBjdND Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="kvm-69-qemu-ide-dvdrom.patch" --- kvm-69/qemu/hw/ide.c 2008-05-12 04:30:43.000000000 -0700 +++ kvm-69/qemu/hw/ide.c 2008-05-25 01:44:27.000000000 -0700 @@ -1716,6 +1716,7 @@ case GPCMD_GET_CONFIGURATION: { uint32_t len; + uint8_t index = 0; /* only feature 0 is supported */ if (packet[2] != 0 || packet[3] != 0) { @@ -1726,41 +1727,40 @@ /* XXX: could result in alignment problems in some architectures */ max_len = ube16_to_cpu(packet + 7); - /* - * XXX: avoid overflow for io_buffer if max_len is bigger than the - * size of that buffer (dimensioned to max number of sectors - * to transfer at once) - * - * Only a problem if the feature/profiles grow exponentially. - */ - if (max_len > 512) /* XXX: assume 1 sector */ - max_len = 512; - - memset(buf, 0, max_len); - /* - * the number of sectors from the media tells us which profile - * to use as current. 0 means there is no media - * - * XXX: fails to detect correctly DVDs with less data burned - * than what a CD can hold - */ - if ((s -> nb_sectors)) { - if ((s -> nb_sectors > CD_MAX_SECTORS)) - cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM); - else - cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM); - } - len = 8; /* header completed */ - if (max_len > len) { - uint8_t index = 0; - - buf[10] = 0x02 | 0x01; /* persistent and current */ - len += 4; /* header */ - len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM); - len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM); - } - cpu_to_ube32(buf, len - 4); /* data length */ + if (max_len > 0) { + /* + * XXX: avoid overflow for io_buffer if max_len is bigger than + * the size of that buffer (dimensioned to max number of + * sectors to transfer at once) + * + * Only a problem if the feature/profiles grow + */ + if (max_len > 512) /* XXX: assume 1 sector */ + max_len = 512; + + memset(buf, 0, max_len); + /* + * the number of sectors from the media tells us which profile + * to use as current. 0 means there is no media + * + * XXX: fails to detect correctly DVDs with less data burned + * than what a CD can hold + */ + if (s -> nb_sectors) { + if (s -> nb_sectors > CD_MAX_SECTORS) + cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM); + else + cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM); + } + + buf[10] = 0x02 | 0x01; /* persistent and current */ + len = 12; /* headers: 8 + 4 */ + len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM); + len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM); + cpu_to_ube32(buf, len - 4); /* data length */ + } else + len = 0; ide_atapi_cmd_reply(s, len, max_len); break; --d6Gm4EdcadzBjdND--