From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57736) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XylV9-0004TI-1k for qemu-devel@nongnu.org; Wed, 10 Dec 2014 12:56:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XylV2-0006o9-UL for qemu-devel@nongnu.org; Wed, 10 Dec 2014 12:56:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44916) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XylV2-0006o1-Mo for qemu-devel@nongnu.org; Wed, 10 Dec 2014 12:56:32 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sBAHuUej019702 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 10 Dec 2014 12:56:31 -0500 Message-ID: <5488894B.4020207@redhat.com> Date: Wed, 10 Dec 2014 12:56:27 -0500 From: John Snow MIME-Version: 1.0 References: <1417719559-26380-1-git-send-email-jsnow@redhat.com> <20141209103504.GC18649@stefanha-thinkpad.redhat.com> In-Reply-To: <20141209103504.GC18649@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com On 12/09/2014 05:35 AM, Stefan Hajnoczi wrote: > On Thu, Dec 04, 2014 at 01:59:19PM -0500, John Snow wrote: >> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >> index c63b7e5..d27c34b 100644 >> --- a/hw/ide/atapi.c >> +++ b/hw/ide/atapi.c >> @@ -622,19 +622,98 @@ static void cmd_request_sense(IDEState *s, uint8_t *buf) >> static void cmd_inquiry(IDEState *s, uint8_t *buf) >> { >> int max_len = buf[4]; > > What guarantees do we have about the size of buf in this function? > > We can't trust max_len because it comes from the guest. > buf points to io_buffer, which is always 2048 bytes; the guest can only specify up to 255 here, so we should always be okay truncating at the limit requested by the guest. If the ambiguity of what buf is makes you nervous (say, for instance, some other user of this function in the future uses a buffer that is not s->io_buffer) I think we'd have to add another argument. >> + int idx = 0; >> >> - buf[0] = 0x05; /* CD-ROM */ >> - buf[1] = 0x80; /* removable */ >> - buf[2] = 0x00; /* ISO */ >> - buf[3] = 0x21; /* ATAPI-2 (XXX: put ATAPI-4 ?) */ >> - buf[4] = 31; /* additional length */ >> - buf[5] = 0; /* reserved */ >> - buf[6] = 0; /* reserved */ >> - buf[7] = 0; /* reserved */ >> - padstr8(buf + 8, 8, "QEMU"); >> - padstr8(buf + 16, 16, "QEMU DVD-ROM"); >> - padstr8(buf + 32, 4, s->version); >> - ide_atapi_cmd_reply(s, 36, max_len); >> + /* If the EVPD (Enable Vital Product Data) bit is set in byte 1, >> + * we are being asked for a specific page of info indicated by byte 2. */ >> + if (buf[1] & 0x01) { >> + switch (buf[2]) { >> + case 0x00: >> + /* Supported Pages */ >> + buf[idx++] = 0x05; /* CD-ROM */ >> + buf[idx++] = 0x00; /* Page Code (0x00) */ >> + buf[idx++] = 0x00; /* reserved */ >> + buf[idx++] = 0x02; /* Two pages supported: */ >> + buf[idx++] = 0x00; /* 0x00: Supported Pages, and: */ >> + buf[idx++] = 0x83; /* 0x83: Device Identification. */ >> + break; >> + case 0x83: >> + /* Device Identification. Each entry is optional, but the entries >> + * included here are modeled after libata's VPD responses. */ >> + buf[idx++] = 0x05; /* CD-ROM */ >> + buf[idx++] = 0x83; /* Page Code */ >> + buf[idx++] = 0x00; >> + buf[idx++] = 0x00; /* Length of encapsulated structure: */ >> + >> + /* Entry 1: Serial */ >> + if (idx + 24 > max_len) { >> + /* Not enough room for even the first entry: */ >> + /* 4 byte header + 20 byte string */ >> + ide_atapi_cmd_error(s, ILLEGAL_REQUEST, >> + ASC_DATA_PHASE_ERROR); > > Missing return > >> + } >> + buf[idx++] = 0x02; /* Ascii */ >> + buf[idx++] = 0x00; /* Vendor Specific */ >> + buf[idx++] = 0x00; >> + buf[idx++] = 20; /* Remaining length */ >> + padstr8(buf + idx, 20, s->drive_serial_str); >> + idx += 20; >> + >> + /* Entry 2: Drive Model and Serial */ >> + if (idx + 72 > max_len) { >> + /* 4 (header) + 8 (vendor) + 60 (model & serial) */ >> + goto out; >> + } > > I guess this time it's okay to return early when there is not enough > space left due to optional fields? > Yes, my goal was "best effort" at giving useful data. > Do we need to set buf[3] (which is currently 0)? > Yes. V2 incoming...