Re: [Qemu-devel] [PULL 4/5] usb-mtp: Introduce write support for MTP objects

[Peter Maydell]

On 27 February 2018 at 08:39, Gerd Hoffmann <address@hidden> wrote:
> From: Bandan Das <address@hidden>
>
> Allow write operations on behalf of the initiator. The
> precursor to write is the sending of the write metadata
> that consists of the ObjectInfo dataset. This patch introduces
> a flag that is set when the responder is ready to receive
> write data based on a previous SendObjectInfo operation by
> the initiator (The SendObjectInfo implementation is in a
> later patch)
>
> Signed-off-by: Bandan Das <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Gerd Hoffmann <address@hidden>

Hi. Coverity (CID 1390604) complains here about a possible
use-after-NULL-check:

> @@ -1567,8 +1706,13 @@ static void usb_mtp_handle_data(USBDevice *dev, 
> USBPacket *p)
>              p->status = USB_RET_STALL;
>              return;
>          }
> -        usb_packet_copy(p, &container, sizeof(container));
> -        switch (le16_to_cpu(container.type)) {
> +        if (s->data_out && !s->data_out->first) {
> +            container_type = TYPE_DATA;

Here we check s->data_out, so we might set container_type
to TYPE_DATA with s->data_out == NULL...

> +        } else {
> +            usb_packet_copy(p, &container, sizeof(container));
> +            container_type = le16_to_cpu(container.type);
> +        }
> +        switch (container_type) {
>          case TYPE_COMMAND:
>              if (s->data_in || s->data_out || s->result) {
>                  trace_usb_mtp_stall(s->dev.addr, "transaction inflight");
> @@ -1599,6 +1743,15 @@ static void usb_mtp_handle_data(USBDevice *dev, 
> USBPacket *p)
>                                    (cmd.argc > 4) ? cmd.argv[4] : 0);
>              usb_mtp_command(s, &cmd);
>              break;
> +        case TYPE_DATA:
> +            /* One of the previous transfers has already errored but the
> +             * responder is still sending data associated with it
> +             */
> +            if (s->result != NULL) {
> +                return;
> +            }
> +            usb_mtp_get_data(s, &container, p);

...but here we call usb_mtp_get_data(), which will always
dereference s->data_out, and so will crash if it is NULL.

> +            break;
>          default:
>              /* not needed as long as the mtp device is read-only */
>              p->status = USB_RET_STALL;
> --
> 2.9.3

thanks
-- PMM