| Message ID | 1417719559-26380-1-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > + 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? Do we need to set buf[3] (which is currently 0)?
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...
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]; + 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); + } + 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; + } + buf[idx++] = 0x02; /* Ascii */ + buf[idx++] = 0x01; /* T10 Vendor */ + buf[idx++] = 0x00; + buf[idx++] = 68; + padstr8(buf + idx, 8, "ATA"); /* Generic T10 vendor */ + idx += 8; + padstr8(buf + idx, 40, s->drive_model_str); + idx += 40; + padstr8(buf + idx, 20, s->drive_serial_str); + idx += 20; + + /* Entry 3: WWN */ + if (s->wwn && (idx + 12 <= max_len)) { + /* 4 byte header + 8 byte wwn */ + buf[idx++] = 0x01; /* Binary */ + buf[idx++] = 0x03; /* NAA */ + buf[idx++] = 0x00; + buf[idx++] = 0x08; + stq_be_p(&buf[idx], s->wwn); + idx += 8; + } + + /* Size of the keys we added, minus the header: */ + buf[3] = idx - 4; + + break; + default: + /* SPC-3, revision 23 sec. 6.4 */ + ide_atapi_cmd_error(s, ILLEGAL_REQUEST, + ASC_INV_FIELD_IN_CMD_PACKET); + return; + } + } else { + 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); + idx = 36; + } + + out: + ide_atapi_cmd_reply(s, idx, max_len); + return; } static void cmd_get_configuration(IDEState *s, uint8_t *buf) diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 8a3eca4..c998003 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -296,6 +296,7 @@ typedef struct IDEDMAOps IDEDMAOps; #define ASC_INCOMPATIBLE_FORMAT 0x30 #define ASC_MEDIUM_NOT_PRESENT 0x3a #define ASC_SAVING_PARAMETERS_NOT_SUPPORTED 0x39 +#define ASC_DATA_PHASE_ERROR 0x4b #define ASC_MEDIA_REMOVAL_PREVENTED 0x53 #define CFA_NO_ERROR 0x00
SCSI devices have multiple kinds of queries they need to respond to, as defined in the "cmd inquiry" section in MMC-6 and SPC-3. Relevent sections: MMC-6 revision 2g: Non-VPD response data and pointer to SPC-3; Section 6.8 "Inquiry Command" SPC-3 revision 23: Inquiry command and error handling: Section 6.4 "INQUIRY command" VPD data pages format: Section 7.6 "Vital product data parameters" We implement these Vital Product Data queries for SCSI, but not for ATAPI through IDE. The result is that if you are looking for the WWN identifier via tools such as sg3_utils, you will be unable to query our CD/DVD rom device to obtain it. This patch adds the minimum number of mandatory responses as defined by SPC-3, which include the "supported pages" response (page 0x00) and the "Device Identification" response (page 0x83). It also correctly responds when it receives a request for an illegal page to improve error output from related tools. The Device ID page contains an arbitrary list of identification strings of various formats; the ID strings included in this patch were chosen to mimic those provided by the libata driver when emulating this SCSI query (model, serial, and wwn when present.) Example: # libata emulated response [root@localhost ~]# sg_inq --id /dev/sda VPD INQUIRY: Device Identification page Designation descriptor number 1, descriptor length: 24 designator_type: vendor specific [0x0], code_set: ASCII associated with the addressed logical unit vendor specific: QM00001 Designation descriptor number 2, descriptor length: 72 designator_type: T10 vendor identification, code_set: ASCII associated with the addressed logical unit vendor id: ATA vendor specific: QEMU HARDDISK QM00001 # QEMU generated ATAPI response, with WWN [root@localhost ~]# sg_inq --id /dev/sr0 VPD INQUIRY: Device Identification page Designation descriptor number 1, descriptor length: 24 designator_type: vendor specific [0x0], code_set: ASCII associated with the addressed logical unit vendor specific: QM00005 Designation descriptor number 2, descriptor length: 72 designator_type: T10 vendor identification, code_set: ASCII associated with the addressed logical unit vendor id: ATA vendor specific: QEMU DVD-ROM QM00005 Designation descriptor number 3, descriptor length: 12 designator_type: NAA, code_set: Binary associated with the addressed logical unit NAA 5, IEEE Company_id: 0xc50 Vendor Specific Identifier: 0x15ea71bb [0x5000c50015ea71bb] See also: hw/scsi/scsi-disk.c, scsi_disk_emulate_inquiry() Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/atapi.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++------- hw/ide/internal.h | 1 + 2 files changed, 92 insertions(+), 12 deletions(-)