Re: [Qemu-devel] [PATCH] ide: Implement VPD response for ATAPI

[Stefan Hajnoczi]

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)?

pgppWLd3gcoxM.pgp
Description: PGP signature