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 <felipe@nutanix.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
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
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
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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> 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);
>
BTW, this is okay even without the follow-up, since libiscsi seems not
to obey its contract...
Paolo
On Fri, Feb 09, 2018 at 06:50:06PM +0100, Paolo Bonzini wrote:
> 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 <felipe@nutanix.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > 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);
> >
>
> BTW, this is okay even without the follow-up, since libiscsi seems not
> to obey its contract...
I'm awaiting feedback from Felipe to see if these fixes plus the
follow-up have solved the issue. Then I'll send a new revision.
Stefan
> On 3 Feb 2018, at 06:16, Stefan Hajnoczi <stefanha@redhat.com> 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 <felipe@nutanix.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Felipe Franciosi <felipe@nutanix.com>
Tested-by: Sreejith Mohanan <sreejit.mohanan@nutanix.com>
> ---
> 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
>
© 2016 - 2026 Red Hat, Inc.