Re: [Qemu-block] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free

[Felipe Franciosi]

> On 3 Feb 2018, at 06:16, Stefan Hajnoczi <address@hidden> wrote:
> 
> 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: Felipe Franciosi <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>

Reviewed-by: Felipe Franciosi <address@hidden>
Tested-by: Sreejith Mohanan <address@hidden>

> ---
> block/iscsi.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1cfe1c647c..8140baac15 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -119,6 +119,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 */
> @@ -282,6 +283,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)
> @@ -290,6 +292,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
> @@ -298,14 +301,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 = {
> @@ -1000,6 +1014,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);
> -- 
> 2.14.3
>