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)?
pgpg9v5wlC00F.pgp
Description: PGP signature
