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

[Bandan Das <bsd@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 27 February 2018 at 08:39, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> From: Bandan Das <bsd@redhat.com>
>>
>> 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 <bsd@redhat.com>
>> Message-id: 20180223164829.29683-5-bsd@redhat.com
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> 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...

Just to be sure, is it enough to replace the if with:
if (s->data !=NULL && !s->data->first) ?

Bandan

>> +        } 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