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

[Stefan Hajnoczi]

On Wed, Apr 23, 2014 at 10:31:01AM +0200, Gerd Hoffmann wrote:

Just a quick review.  If I understand correctly, the guest never sends
filenames to the guest.  Instead filenames are discovered using readdir
inside QEMU and the guest accesses objects by handle.  This seems like a
good property for security since it eliminates '..' escaping attacks.

> +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);

Use a trace event instead or drop this?

> +static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
> +{
> +    struct dirent *entry;
> +    DIR *dir;
> +
> +    o->nchildren = 0;
> +    dir = opendir(o->path);
> +    if (!dir) {
> +        return;
> +    }
> +    while ((entry = readdir(dir)) != NULL) {

Please use the reentrant readdir_r() so we don't have to worry later on
when removing the QEMU global mutex.

> +    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__);
> +                }

Please flesh this out and loop for EINTR.

> +    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);
> +            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);

We should not print guest data to stderr in production since guests can
be malicious and this DOSes the logs.