Re: [Qemu-devel] [PULL 5/5] usb-mtp: Advertise SendObjectInfo for write support

[Peter Maydell]

On 27 February 2018 at 08:39, Gerd Hoffmann <address@hidden> wrote:
> From: Bandan Das <address@hidden>
>
> This patch implements a dummy ObjectInfo structure so that
> it's easy to typecast the incoming data. If the metadata is
> valid, write_pending is set. Also, the incoming filename
> is utf-16, so, instead of depending on external libraries, just
> implement a simple function to get the filename
>
> Signed-off-by: Bandan Das <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  hw/usb/dev-mtp.c | 136 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 2 deletions(-)

Hi; Coverity points out a pointer use-after-NULL-check in this
code (CID1390592):

> +    case CMD_SEND_OBJECT_INFO:
> +        /* Return error if store is read-only */
> +        if (!FLAG_SET(s, MTP_FLAG_WRITABLE)) {
> +            usb_mtp_queue_result(s, RES_STORE_READ_ONLY,
> +                                 c->trans, 0, 0, 0, 0);
> +        } else if (c->argv[0] && (c->argv[0] != QEMU_STORAGE_ID)) {
> +            /* First parameter points to storage id or is 0 */
> +            usb_mtp_queue_result(s, RES_STORE_NOT_AVAILABLE, c->trans,
> +                                 0, 0, 0, 0);
> +        } else if (c->argv[1] && !c->argv[0]) {
> +            /* If second parameter is specified, first must also be 
> specified */
> +            usb_mtp_queue_result(s, RES_DESTINATION_UNSUPPORTED, c->trans,
> +                                 0, 0, 0, 0);
> +        } else {
> +            uint32_t handle = c->argv[1];
> +            if (handle == 0xFFFFFFFF || handle == 0) {
> +                /* root object */
> +                o = QTAILQ_FIRST(&s->objects);
> +            } else {
> +                o = usb_mtp_object_lookup(s, handle);
> +            }
> +            if (o == NULL) {

Here we do a NULL check on 'o'...

> +                usb_mtp_queue_result(s, RES_INVALID_OBJECT_HANDLE, c->trans,
> +                                     0, 0, 0, 0);
> +            }
> +            if (o->format != FMT_ASSOCIATION) {

...but here we dereference 'o', which will crash if it is NULL.

> +                usb_mtp_queue_result(s, RES_INVALID_PARENT_OBJECT, c->trans,
> +                                     0, 0, 0, 0);
> +            }
> +        }
> +        if (o) {
> +            s->dataset.parent_handle = o->handle;
> +        }
> +        s->data_out = usb_mtp_data_alloc(c);
> +        return;

thanks
-- PMM