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:
> 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 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  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(>mutex);
> +
> +/* If it was cancelled or completed already, our work is done here */
> +if (acb->cancelled || acb->status != -EINPROGRESS) {
> +qemu_mutex_unlock(>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(>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);
> 

BTW, this is okay even without the follow-up, since libiscsi seems not
to obey its contract...

Paolo

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(>mutex);
> +
> +/* If it was cancelled or completed already, our work is done here */
> +if (acb->cancelled || acb->status != -EINPROGRESS) {
> +qemu_mutex_unlock(>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

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

[Stefan Hajnoczi]

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 
Signed-off-by: Stefan Hajnoczi 
---
 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(>mutex);
+
+/* If it was cancelled or completed already, our work is done here */
+if (acb->cancelled || acb->status != -EINPROGRESS) {
+qemu_mutex_unlock(>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(>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