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

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

[Stefan Hajnoczi]

Patches 1 & 2 are cleanups.

Patch 3 fixes cancellation of ioctls.  Felipe showed me a trace where an acb is
cancelled and then completes twice.  The second time around crashes QEMU.

Compile-tested only.

Felipe: Please let us know if this fixes the issue you are seeing.  Thanks!

Stefan Hajnoczi (3):
  block/iscsi: drop unused IscsiAIOCB->buf field
  block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()
  block/iscsi: fix ioctl cancel use-after-free

 block/iscsi.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] block/iscsi: drop unused IscsiAIOCB->buf field

[Stefan Hajnoczi]

The IscsiAIOCB->buf field has not been used since commit
e49ab19fcaa617ad6cdfe1ac401327326b6a2552 ("block/iscsi: bump libiscsi
requirement to 1.9.0").  It used to be a linear buffer for old libiscsi
versions that didn't support scatter-gather.  The minimum libiscsi
version supports scatter-gather so we don't linearize buffers anymore.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6a1c53711a..cd0738942c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -112,7 +112,6 @@ typedef struct IscsiAIOCB {
     QEMUBH *bh;
     IscsiLun *iscsilun;
     struct scsi_task *task;
-    uint8_t *buf;
     int status;
     int64_t sector_num;
     int nb_sectors;
@@ -145,9 +144,6 @@ iscsi_bh_cb(void *p)
 
     qemu_bh_delete(acb->bh);
 
-    g_free(acb->buf);
-    acb->buf = NULL;
-
     acb->common.cb(acb->common.opaque, acb->status);
 
     if (acb->task != NULL) {
@@ -925,9 +921,6 @@ iscsi_aio_ioctl_cb(struct iscsi_context *iscsi, int status,
 {
     IscsiAIOCB *acb = opaque;
 
-    g_free(acb->buf);
-    acb->buf = NULL;
-
     acb->status = 0;
     if (status < 0) {
         error_report("Failed to ioctl(SG_IO) to iSCSI lun. %s",
@@ -1002,7 +995,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState *bs,
     acb->iscsilun = iscsilun;
     acb->bh          = NULL;
     acb->status      = -EINPROGRESS;
-    acb->buf         = NULL;
     acb->ioh         = buf;
 
     if (req != SG_IO) {
-- 
2.14.3

[Qemu-devel] [PATCH 2/3] block/iscsi: take iscsilun->mutex in iscsi_timed_check_events()

[Stefan Hajnoczi]

Commit d045c466d9e62b4321fadf586d024d54ddfd8bd4 ("iscsi: do not use
aio_context_acquire/release") introduced iscsilun->mutex but appears to
have overlooked iscsi_timed_check_events() when introducing the mutex.

iscsi_service() and iscsi_set_events() must be called with
iscsilun->mutex held.

iscsi_timed_check_events() is invoked from the AioContext and does not
take the mutex.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index cd0738942c..1cfe1c647c 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -339,6 +339,8 @@ static void iscsi_timed_check_events(void *opaque)
 {
     IscsiLun *iscsilun = opaque;
 
+    qemu_mutex_lock(&iscsilun->mutex);
+
     /* check for timed out requests */
     iscsi_service(iscsilun->iscsi, 0);
 
@@ -351,6 +353,8 @@ static void iscsi_timed_check_events(void *opaque)
      * to return to service once this situation changes. */
     iscsi_set_events(iscsilun);
 
+    qemu_mutex_unlock(&iscsilun->mutex);
+
     timer_mod(iscsilun->event_timer,
               qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + EVENT_INTERVAL);
 }
-- 
2.14.3

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

[Stefan Hajnoczi]

The ioctl request cancellation code assumes that requests do not
complete once TASK ABORT has been sent to the iSCSI target.  The request
completion callback is unconditionally invoked when TASK ABORT finishes.
Therefore the request completion callback is invoked twice if the
request does happen to complete before TASK ABORT.

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

The iscsilun->mutex protection is also missing in iscsi_aio_cancel().

This patch rewrites iscsi_aio_cancel() and iscsi_abort_task_cb() to
avoid double completion, use-after-free, and to take iscsilun->mutex
when needed.

Reported-by: Felipe Franciosi <felipe@nutanix.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/iscsi.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 1cfe1c647c..4566902d43 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -282,14 +282,19 @@ 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)
 {
     IscsiAIOCB *acb = private_data;
 
-    acb->status = -ECANCELED;
-    iscsi_schedule_bh(acb);
+    /* Skip if the request already completed */
+    if (acb->status == -ECANCELED) {
+        iscsi_schedule_bh(acb);
+    }
+
+    qemu_aio_unref(acb); /* acquired in iscsi_aio_cancel() */
 }
 
 static void
@@ -298,14 +303,26 @@ iscsi_aio_cancel(BlockAIOCB *blockacb)
     IscsiAIOCB *acb = (IscsiAIOCB *)blockacb;
     IscsiLun *iscsilun = acb->iscsilun;
 
+    qemu_mutex_lock(&iscsilun->mutex);
+
+    /* If it was cancelled or completed already, our work is done here */
     if (acb->status != -EINPROGRESS) {
+        qemu_mutex_unlock(&iscsilun->mutex);
         return;
     }
 
+    /* This can still be overwritten if the request completes */
+    acb->status = -ECANCELED;
+
+    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 = {
-- 
2.14.3

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

[Stefan Hajnoczi]

On Fri, Feb 2, 2018 at 10:16 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The ioctl request cancellation code assumes that requests do not
> complete once TASK ABORT has been sent to the iSCSI target.  The request
> completion callback is unconditionally invoked when TASK ABORT finishes.
> Therefore the request completion callback is invoked twice if the
> request does happen to complete before TASK ABORT.
>
> Futhermore, iscsi_aio_cancel() does not increment the request's
> reference count, causing a use-after-free when TASK ABORT finishes after
> the request has already completed.
>
> The iscsilun->mutex protection is also missing in iscsi_aio_cancel().
>
> This patch rewrites iscsi_aio_cancel() and iscsi_abort_task_cb() to
> avoid double completion, use-after-free, and to take iscsilun->mutex
> when needed.
>
> Reported-by: Felipe Franciosi <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/iscsi.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 1cfe1c647c..4566902d43 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -282,14 +282,19 @@ 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)
>  {
>      IscsiAIOCB *acb = private_data;
>
> -    acb->status = -ECANCELED;
> -    iscsi_schedule_bh(acb);
> +    /* Skip if the request already completed */
> +    if (acb->status == -ECANCELED) {

There is a bug here.  acb->status can be -ECANCELED if the request
completed with COMMAND ABORTED.

I'll send a v2 of this patch tomorrow to solve this.

Stefan