Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free
> On 3 Feb 2018, at 06: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 Reviewed-by: Felipe Franciosi Tested-by: Sreejith Mohanan > --- > 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 >
Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free
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 > > 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(&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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v2 3/3] block/iscsi: fix ioctl cancel use-after-free
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(&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