Re: [Qemu-devel] [PULL v2 2/2] usb: mtp filesharing

[Peter Wu]

Hi,

For this review, I grabbed the MTP 1.1 spec from USB.org. There are some
issues which are noted below.

On Wednesday 23 April 2014 10:31:01 Gerd Hoffmann wrote:
> Implementation of a USB Media Transfer Device device for easy
> filesharing.  Read-only.  No access control inside qemu, it will
> happily export any file it is able to open to the guest, i.e.
> standard unix access rights for the qemu process apply.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  default-configs/usb.mak |    1 +
>  hw/usb/Makefile.objs    |    4 +
>  hw/usb/dev-mtp.c        | 1103 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  trace-events            |   21 +
>  4 files changed, 1129 insertions(+)
>  create mode 100644 hw/usb/dev-mtp.c

<snip>

> +/* ----------------------------------------------------------------------- */
> +
> +static MTPObject *usb_mtp_object_alloc(MTPState *s, uint32_t handle,
> +                                       MTPObject *parent, char *name)
> +{
> +    MTPObject *o = g_new0(MTPObject, 1);
> +
> +    if (name[0] == '.') {
> +        goto ignore;
> +    }
> +
> +    o->handle = handle;
> +    o->parent = parent;
> +    o->name = g_strdup(name);
> +    o->nchildren = -1;
> +    if (parent == NULL) {
> +        o->path = g_strdup(name);
> +    } else {
> +        o->path = g_strdup_printf("%s/%s", parent->path, name);
> +    }
> +
> +    if (lstat(o->path, &o->stat) != 0) {
> +        goto ignore;
> +    }
> +    if (S_ISREG(o->stat.st_mode)) {
> +        o->format = FMT_UNDEFINED_OBJECT;
> +    } else if (S_ISDIR(o->stat.st_mode)) {
> +        o->format = FMT_ASSOCIATION;
> +    } else {
> +        goto ignore;
> +    }
> +
> +    if (access(o->path, R_OK) != 0) {
> +        goto ignore;
> +    }
> +
> +    fprintf(stderr, "%s: 0x%x %s\n", __func__, o->handle, o->path);

Debugging left-over?

> +    QTAILQ_INSERT_TAIL(&s->objects, o, next);
> +    return o;
> +
> +ignore:
> +    g_free(o->name);
> +    g_free(o->path);
> +    g_free(o);
> +    return NULL;
> +}
> +
> +static void usb_mtp_object_free(MTPState *s, MTPObject *o)
> +{
> +    int i;
> +
> +    fprintf(stderr, "%s: 0x%x %s\n", __func__, o->handle, o->path);

Debugging left-over #2?

> +    QTAILQ_REMOVE(&s->objects, o, next);
> +    for (i = 0; i < o->nchildren; i++) {
> +        usb_mtp_object_free(s, o->children[i]);
> +    }
> +    g_free(o->children);
> +    g_free(o->name);
> +    g_free(o->path);
> +    g_free(o);
> +}
> +

<snip>

> +static void usb_mtp_add_u8(MTPData *data, uint8_t val)
> +{
> +    usb_mtp_realloc(data, 1);
> +    data->data[data->length++] = val;
> +}
> +
> +static void usb_mtp_add_u16(MTPData *data, uint16_t val)
> +{
> +    usb_mtp_realloc(data, 2);
> +    data->data[data->length++] = (val >> 0) & 0xff;
> +    data->data[data->length++] = (val >> 8) & 0xff;
> +}
> +
> +static void usb_mtp_add_u32(MTPData *data, uint32_t val)
> +{
> +    usb_mtp_realloc(data, 4);
> +    data->data[data->length++] = (val >>  0) & 0xff;
> +    data->data[data->length++] = (val >>  8) & 0xff;
> +    data->data[data->length++] = (val >> 16) & 0xff;
> +    data->data[data->length++] = (val >> 24) & 0xff;
> +}
> +
> +static void usb_mtp_add_u64(MTPData *data, uint64_t val)
> +{
> +    usb_mtp_realloc(data, 4);

usb_mtp_realloc(data, 8);

> +    data->data[data->length++] = (val >>  0) & 0xff;
> +    data->data[data->length++] = (val >>  8) & 0xff;
> +    data->data[data->length++] = (val >> 16) & 0xff;
> +    data->data[data->length++] = (val >> 24) & 0xff;
> +    data->data[data->length++] = (val >> 32) & 0xff;
> +    data->data[data->length++] = (val >> 40) & 0xff;
> +    data->data[data->length++] = (val >> 48) & 0xff;
> +    data->data[data->length++] = (val >> 54) & 0xff;

48 + 8 = 56. What about a loop instead?

> +}
> +

<snip>

> +/* ----------------------------------------------------------------------- */
> +
> +static MTPData *usb_mtp_get_device_info(MTPState *s, MTPControl *c)
> +{
> +    static const uint16_t ops[] = {
> +        CMD_GET_DEVICE_INFO,
> +        CMD_OPEN_SESSION,
> +        CMD_CLOSE_SESSION,
> +        CMD_GET_STORAGE_IDS,
> +        CMD_GET_STORAGE_INFO,
> +        CMD_GET_NUM_OBJECTS,
> +        CMD_GET_OBJECT_HANDLES,
> +        CMD_GET_OBJECT_INFO,
> +        CMD_GET_OBJECT,
> +        CMD_GET_PARTIAL_OBJECT,
> +    };
> +    static const uint16_t fmt[] = {
> +        FMT_UNDEFINED_OBJECT,
> +        FMT_ASSOCIATION,
> +    };
> +    MTPData *d = usb_mtp_data_alloc(c);
> +
> +    trace_usb_mtp_op_get_device_info(s->dev.addr);
> +
> +    usb_mtp_add_u16(d, 0x0100);

Sect. 5.1.1.1 says:

    "This identifies the PTP version this device can support in
    hundredths.  For MTP devices implemented under this specification,
    this shall contain the value 100 (representing 1.00)."

Is it an error in the spec (missing 0x) or should the value here really
be 0x0100 instead of 0x0064?


> +    usb_mtp_add_u32(d, 0xffffffff);
> +    usb_mtp_add_u16(d, 0x0101);
> +    usb_mtp_add_wstr(d, L"");
> +    usb_mtp_add_u16(d, 0x0000);
> +
> +    usb_mtp_add_u16_array(d, ARRAY_SIZE(ops), ops);
> +    usb_mtp_add_u16_array(d, 0, NULL);
> +    usb_mtp_add_u16_array(d, 0, NULL);
> +    usb_mtp_add_u16_array(d, 0, NULL);
> +    usb_mtp_add_u16_array(d, ARRAY_SIZE(fmt), fmt);
> +
> +    usb_mtp_add_wstr(d, L"" MTP_MANUFACTURER);
> +    usb_mtp_add_wstr(d, L"" MTP_PRODUCT);
> +    usb_mtp_add_wstr(d, L"0.1");
> +    usb_mtp_add_wstr(d, L"123456789abcdef123456789abcdef");

This serial number is 30 characters, you skipped the zeroes. According
to sect 5.1.1.14, it "shall be exactly 32 characters, including any
leading 0s".

> +
> +    return d;
> +}

<snip>

> +static MTPData *usb_mtp_get_object(MTPState *s, MTPControl *c,
> +                                   MTPObject *o)
> +{
> +    MTPData *d = usb_mtp_data_alloc(c);
> +
> +    trace_usb_mtp_op_get_object(s->dev.addr, o->handle, o->path);
> +
> +    d->fd = open(o->path, O_RDONLY);
> +    if (d->fd == -1) {

usb_mtp_data_free(d);

> +        return NULL;
> +    }
> +    d->length = o->stat.st_size;
> +    d->alloc  = 512;
> +    d->data   = g_malloc(d->alloc);
> +    return d;
> +}
> +
> +static MTPData *usb_mtp_get_partial_object(MTPState *s, MTPControl *c,
> +                                           MTPObject *o)
> +{
> +    MTPData *d = usb_mtp_data_alloc(c);
> +    off_t offset;
> +
> +    trace_usb_mtp_op_get_partial_object(s->dev.addr, o->handle, o->path,
> +                                        c->argv[1], c->argv[2]);
> +
> +    d->fd = open(o->path, O_RDONLY);
> +    if (d->fd == -1) {

usb_mtp_data_free(d);

> +        return NULL;
> +    }
> +
> +    offset = c->argv[1];
> +    if (offset > o->stat.st_size) {
> +        offset = o->stat.st_size;
> +    }
> +    lseek(d->fd, offset, SEEK_SET);
> +
> +    d->length = c->argv[2];
> +    if (d->length > o->stat.st_size - offset) {
> +        d->length = o->stat.st_size - offset;
> +    }
> +
> +    return d;
> +}

<snip>

> +static void usb_mtp_handle_data(USBDevice *dev, USBPacket *p)
> +{
> +    MTPState *s = DO_UPCAST(MTPState, dev, dev);
> +    MTPControl cmd;
> +    mtp_container container;
> +    uint32_t params[5];
> +    int i, rc;
> +
> +    switch (p->ep->nr) {
> +    case EP_DATA_IN:
> +        if (s->data_out != NULL) {
> +            /* guest bug */
> +            trace_usb_mtp_stall(s->dev.addr, "awaiting data-out");
> +            p->status = USB_RET_STALL;
> +            return;
> +        }
> +        if (p->iov.size < sizeof(container)) {
> +            trace_usb_mtp_stall(s->dev.addr, "packet too small");
> +            p->status = USB_RET_STALL;
> +            return;
> +        }
> +        if (s->data_in !=  NULL) {
> +            MTPData *d = s->data_in;
> +            int dlen = d->length - d->offset;
> +            if (d->first) {
> +                trace_usb_mtp_data_in(s->dev.addr, d->trans, d->length);
> +                container.length = cpu_to_le32(d->length + 
> sizeof(container));
> +                container.type   = cpu_to_le16(TYPE_DATA);
> +                container.code   = cpu_to_le16(d->code);
> +                container.trans  = cpu_to_le32(d->trans);
> +                usb_packet_copy(p, &container, sizeof(container));
> +                d->first = false;
> +                if (dlen > p->iov.size - sizeof(container)) {
> +                    dlen = p->iov.size - sizeof(container);
> +                }
> +            } else {
> +                if (dlen > p->iov.size) {
> +                    dlen = p->iov.size;
> +                }
> +            }
> +            if (d->fd == -1) {
> +                usb_packet_copy(p, d->data + d->offset, dlen);
> +            } else {
> +                if (d->alloc < p->iov.size) {
> +                    d->alloc = p->iov.size;
> +                    d->data = g_realloc(d->data, d->alloc);
> +                }
> +                rc = read(d->fd, d->data, dlen);
> +                if (rc != dlen) {
> +                    fprintf(stderr, "%s: TODO: handle read error\n", 
> __func__);

It should probably respond with Incomplete_Transfer (0x2007) per 
section D2.9, F2.8.

> +                }
> +                usb_packet_copy(p, d->data, dlen);
> +            }
> +            d->offset += dlen;
> +            if (d->offset == d->length) {
> +                usb_mtp_data_free(s->data_in);

(ah, s->data_in == d, so no fd leak of d->fd)

> +                s->data_in = NULL;
> +            }
> +        } else if (s->result != NULL) {
> +            MTPControl *r = s->result;
> +            int length = sizeof(container) + r->argc * sizeof(uint32_t);
> +            if (r->code == RES_OK) {
> +                trace_usb_mtp_success(s->dev.addr, r->trans,
> +                                      (r->argc > 0) ? r->argv[0] : 0,
> +                                      (r->argc > 1) ? r->argv[1] : 0);
> +            } else {
> +                trace_usb_mtp_error(s->dev.addr, r->code, r->trans,
> +                                    (r->argc > 0) ? r->argv[0] : 0,
> +                                    (r->argc > 1) ? r->argv[1] : 0);
> +            }
> +            container.length = cpu_to_le32(length);
> +            container.type   = cpu_to_le16(TYPE_RESPONSE);
> +            container.code   = cpu_to_le16(r->code);
> +            container.trans  = cpu_to_le32(r->trans);
> +            for (i = 0; i < r->argc; i++) {
> +                params[i] = cpu_to_le32(r->argv[i]);
> +            }
> +            usb_packet_copy(p, &container, sizeof(container));
> +            usb_packet_copy(p, &params, length - sizeof(container));
> +            g_free(s->result);
> +            s->result = NULL;
> +        }
> +        break;
> +    case EP_DATA_OUT:
> +        if (p->iov.size < sizeof(container)) {
> +            trace_usb_mtp_stall(s->dev.addr, "packet too small");
> +            p->status = USB_RET_STALL;
> +            return;
> +        }
> +        usb_packet_copy(p, &container, sizeof(container));
> +        switch (le16_to_cpu(container.type)) {
> +        case TYPE_COMMAND:
> +            if (s->data_in || s->data_out || s->result) {
> +                trace_usb_mtp_stall(s->dev.addr, "transaction inflight");
> +                p->status = USB_RET_STALL;
> +                return;
> +            }
> +            cmd.code = le16_to_cpu(container.code);
> +            cmd.argc = (le32_to_cpu(container.length) - sizeof(container))
> +                / sizeof(uint32_t);

This is information passed by the guest, would it make sense to reject
packets with more than five parameters? Or at least restrict cmd.argc?

> +            cmd.trans = le32_to_cpu(container.trans);
> +            usb_packet_copy(p, &params, cmd.argc * sizeof(uint32_t));
> +            for (i = 0; i < cmd.argc; i++) {
> +                cmd.argv[i] = le32_to_cpu(params[i]);
> +            }
> +            trace_usb_mtp_command(s->dev.addr, cmd.code, cmd.trans,
> +                                  (cmd.argc > 0) ? cmd.argv[0] : 0,
> +                                  (cmd.argc > 1) ? cmd.argv[1] : 0,
> +                                  (cmd.argc > 2) ? cmd.argv[2] : 0,
> +                                  (cmd.argc > 3) ? cmd.argv[3] : 0,
> +                                  (cmd.argc > 4) ? cmd.argv[4] : 0);
> +            usb_mtp_command(s, &cmd);
> +            break;
> +        default:
> +            iov_hexdump(p->iov.iov, p->iov.niov, stderr, "mtp-out", 32);
> +            trace_usb_mtp_stall(s->dev.addr, "TODO: implement data-out");
> +            p->status = USB_RET_STALL;
> +            return;
> +        }
> +        break;
> +    case EP_EVENT:
> +        p->status = USB_RET_NAK;
> +        return;
> +    default:
> +        trace_usb_mtp_stall(s->dev.addr, "invalid endpoint");
> +        p->status = USB_RET_STALL;
> +        return;
> +    }
> +
> +    if (p->actual_length == 0) {
> +        trace_usb_mtp_nak(s->dev.addr, p->ep->nr);
> +        p->status = USB_RET_NAK;
> +        return;
> +    } else {
> +        trace_usb_mtp_xfer(s->dev.addr, p->ep->nr, p->actual_length,
> +                           p->iov.size);
> +        return;
> +    }
> +}
> +
> +static int usb_mtp_initfn(USBDevice *dev)
> +{
> +    MTPState *s = DO_UPCAST(MTPState, dev, dev);
> +
> +    usb_desc_create_serial(dev);
> +    usb_desc_init(dev);
> +    QTAILQ_INIT(&s->objects);
> +    if (s->desc == NULL) {
> +        s->desc = strrchr(s->root, '/');
> +        if (s->desc) {

If (for some reason) one wants to show the root directory ("/"), then
the desc is empty. Check for s->desc[0] too?

> +            s->desc = g_strdup(s->desc + 1);
> +        } else {
> +            s->desc = g_strdup("none");
> +        }
> +    }
> +    return 0;
> +}

<snip remainder of dev-mtp.c and tracing, no problems found>

The other parts looks fine.

Kind regards,
Peter