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

[Paolo Bonzini]

On 03/02/2018 07:16, Stefan Hajnoczi wrote:
> @@ -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() */

qemu_aio_ref is not thread safe.  I think this is fine however, since
all qemu_aio_ref/unref calls should happen in the I/O thread.

Regarding your follow-up patch:

> 
> -    acb->status = -ECANCELED;
> -    iscsi_schedule_bh(acb);
> +    /* If the command callback hasn't been called yet, drop the task */
> +    if (!acb->bh) {
> +        /* Call iscsi_aio_ioctl_cb() with SCSI_STATUS_CANCELLED */
> +        iscsi_scsi_cancel_task(iscsi, acb->task);
> +    }
> +

SCSI_STATUS_CANCELLED is a libiscsi addition and should not go past
iscsi_aio_ioctl_cb.  So you'd need something like this:

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..ace6ca900f 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -928,6 +928,14 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi,
     g_free(acb->buf);
     acb->buf = NULL;

+    if (status == SCSI_STATUS_CANCELLED) {
+        if (!acb->bh) {
+            acb->status = -ECANCELED);
+            iscsi_schedule_bh(acb);
+        }
+        return;
+    }
+
     acb->status = 0;
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",

Needless to say, it is really unfortunate that there is no mock iSCSI
server to write tests for. :(

Paolo