Peter Maydell <peter.mayd...@linaro.org> writes:
> On 27 February 2018 at 08:39, Gerd Hoffmann <kra...@redhat.com> wrote:
>> From: Bandan Das <b...@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 <b...@redhat.com>
>> Message-id: 20180223164829.29683-5-...@redhat.com
>> Signed-off-by: Gerd Hoffmann <kra...@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
