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, ¶ms, 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.