Commit c1004487 authored by Stefan Hajnoczi's avatar Stefan Hajnoczi Committed by Paolo Bonzini
Browse files

block/iscsi: fix ioctl cancel use-after-free 



iscsi_aio_cancel() does not increment the request's reference count,
causing a use-after-free when ABORT TASK finishes after the request has
already completed.

There are some additional issues with iscsi_aio_cancel():
1. Several ABORT TASKs may be sent for the same task if
   iscsi_aio_cancel() is invoked multiple times.  It's better to avoid
   this just in case the command identifier is reused.
2. The iscsilun->mutex protection is missing in iscsi_aio_cancel().

Reported-by: default avatarFelipe Franciosi <felipe@nutanix.com>
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20180203061621.7033-4-stefanha@redhat.com>
Reviewed-by: default avatarFelipe Franciosi <felipe@nutanix.com>
Tested-by: default avatarSreejith Mohanan <sreejit.mohanan@nutanix.com>
Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
parent 83d11973

block/iscsi.c

+18 −3
Original line number Diff line number Diff line
@@ -124,6 +124,7 @@ typedef struct IscsiAIOCB { 
#ifdef __linux__

    sg_io_hdr_t *ioh;

#endif

    bool cancelled;

} IscsiAIOCB;



/* libiscsi uses time_t so its enough to process events every second */
@@ -287,6 +288,7 @@ static void iscsi_co_init_iscsitask(IscsiLun *iscsilun, struct IscsiTask *iTask) 
    };

}



/* Called (via iscsi_service) with QemuMutex held. */

static void

iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data,

                    void *private_data)
@@ -295,6 +297,7 @@ iscsi_abort_task_cb(struct iscsi_context *iscsi, int status, void *command_data, 


    acb->status = -ECANCELED;

    iscsi_schedule_bh(acb);

    qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */

}



static void
@@ -303,14 +306,25 @@ iscsi_aio_cancel(BlockAIOCB *blockacb) 
    IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;

    IscsiLun *iscsilun = acb->iscsilun;



    if (acb->status != -EINPROGRESS) {

    qemu_mutex_lock(&iscsilun->mutex);



    /* If it was cancelled or completed already, our work is done here */

    if (acb->cancelled || acb->status != -EINPROGRESS) {

        qemu_mutex_unlock(&iscsilun->mutex);

        return;

    }



    acb->cancelled = true;



    qemu_aio_ref(acb); /* released in iscsi_abort_task_cb() */



    /* send a task mgmt call to the target to cancel the task on the target */

    iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,

                                     iscsi_abort_task_cb, acb);

    if (iscsi_task_mgmt_abort_task_async(iscsilun->iscsi, acb->task,

                                         iscsi_abort_task_cb, acb) < 0) {

        qemu_aio_unref(acb); /* since iscsi_abort_task_cb() won't be called */

    }



    qemu_mutex_unlock(&iscsilun->mutex);

}



static const AIOCBInfo iscsi_aiocb_info = {
@@ -1008,6 +1022,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs, 
    acb->bh          = NULL;

    acb->status      = -EINPROGRESS;

    acb->ioh         = buf;

    acb->cancelled   = false;



    if (req != SG_IO) {

        iscsi_ioctl_handle_emulated(acb, req, buf);